Details

Reviewers

broulik
ngraham
meven

Group Reviewers

Plasma

Summary

The FindProfile class was only for Chrome implemented => removed
Having ProfileBookmarks and Profile class is unnecessary, especially because they are only needed for Chrome
Better error handling

Test Plan

Should work as before

Diff Detail

Repository

R120 Plasma Workspace

Branch

api_inconsistencies (branched from master)

Lint

No Linters Available

Unit

No Unit Test Coverage

Build Status

Buildable 27060
Build 27078: arc lint + arc unit

alex created this revision.May 17 2020, 4:14 PM

Restricted Application added a project: Plasma. · View Herald TranscriptMay 17 2020, 4:14 PM

Restricted Application added a subscriber: plasma-devel. · View Herald Transcript

alex requested review of this revision.May 17 2020, 4:14 PM

Harbormaster completed remote builds in B27009: Diff 83019.May 17 2020, 4:14 PM

Commented code should be removed.

runners/bookmarks/browsers/chromeprofile.h
12	You can move contructor to private section, that will make all things clear, `findAllProfiles` should be the only one allowed creator.

Make constructor private

Harbormaster completed remote builds in B27010: Diff 83028.May 17 2020, 6:08 PM

Create test cases

Harbormaster completed remote builds in B27011: Diff 83029.May 17 2020, 8:10 PM

Change import, formatting

Harbormaster completed remote builds in B27058: Diff 83116.May 22 2020, 3:35 PM

Seems good, two small clean ups

runners/bookmarks/browsers/chromeprofile.cpp
9	`{` on new line
28	Remove

Formatting, remove debug statement

Harbormaster completed remote builds in B27060: Diff 83118.May 22 2020, 5:02 PM

alex marked 2 inline comments as done.May 22 2020, 5:02 PM

Two smalls things.

Seems good to me otherwise.

Give some times to other to have a final say before merging.

runners/bookmarks/browsers/chromeprofile.cpp
33	Perhaps use an iterator instead of copying the keys.
runners/bookmarks/fetchsqlite.cpp
32	Is this used here ?

This revision is now accepted and ready to land.May 23 2020, 2:04 PM

Use iterator, remove unused include

Harbormaster completed remote builds in B27066: Diff 83131.May 23 2020, 3:56 PM

alex marked 2 inline comments as done.May 23 2020, 3:57 PM

@broulik perhaps a review ?

Friendly Ping :-)

Or should I just move this to gitlab?

In D29807#675032, @alex wrote:

Friendly Ping :-)

Or should I just move this to gitlab?

This is close to be approved, no need to move this.

I think this can land @broulik @ngraham ?

runners/bookmarks/browsers/chromeprofile.cpp
6	Is this used ?

meven accepted this revision.Jun 16 2020, 1:43 PM

		Path
M		runners/bookmarks/CMakeLists.txt (4 lines)
M		runners/bookmarks/browserfactory.cpp (5 lines)
M		runners/bookmarks/browsers/chrome.h (6 lines)
M		runners/bookmarks/browsers/chrome.cpp (46 lines)
D	M	runners/bookmarks/browsers/chromefindprofile.h (40 lines)
D	M	runners/bookmarks/browsers/chromefindprofile.cpp (68 lines)
A	M	runners/bookmarks/browsers/chromeprofile.h (34 lines)
A	M	runners/bookmarks/browsers/chromeprofile.cpp (40 lines)
D	M	runners/bookmarks/browsers/findprofile.h (54 lines)
M		runners/bookmarks/faviconfromblob.h (2 lines)
M		runners/bookmarks/faviconfromblob.cpp (17 lines)
M		runners/bookmarks/fetchsqlite.h (2 lines)
M		runners/bookmarks/fetchsqlite.cpp (12 lines)
M		runners/bookmarks/tests/CMakeLists.txt (3 lines)
V		runners/bookmarks/tests/{chrome-config-home/.config → }/chromium/Local State (65 lines)
V		runners/bookmarks/tests/{chrome-config-home/Chrome-Bookmarks-Sample.json → chromium/Default/Bookmarks} (55 lines)
V		runners/bookmarks/tests/{chrome-config-home/Chrome-Bookmarks-SecondProfile.json → chromium/Profile 1/Bookmarks} (20 lines)
V		runners/bookmarks/tests/{chromium/Default/Bookmarks ← chrome-config-home/Chrome-Bookmarks-Sample.json} (55 lines)
V		runners/bookmarks/tests/{ ← chrome-config-home/.config}/chromium/Local State (65 lines)
V		runners/bookmarks/tests/{chromium/Profile 1/Bookmarks ← chrome-config-home/Chrome-Bookmarks-SecondProfile.json} (20 lines)
M		runners/bookmarks/tests/testchromebookmarks.h (16 lines)
M		runners/bookmarks/tests/testchromebookmarks.cpp (82 lines)

Diff	ID	Base	Description	Created	Lint	Unit
Base			Base
Diff 1	83019	8d9dfa6		May 17 2020, 4:14 PM	★	★
Diff 2	83028	8d9dfa6	Make constructor private	May 17 2020, 6:08 PM	★	★
Diff 3	83029	8d9dfa6	Create test cases	May 17 2020, 8:10 PM	★	★
Diff 4	83116	8d9dfa6	Change import, formatting	May 22 2020, 3:35 PM	★	★
Diff 5	83118	8d9dfa6	Formatting, remove debug statement	May 22 2020, 5:02 PM	★	★
Diff 6	83131	8d9dfa6	Use iterator, remove unused include	May 23 2020, 3:56 PM	★	★

Change Chrome API design
AcceptedPublic
Actions

Details

Diff Detail

Revision Contents
Changeset List

Diff 83118

runners/bookmarks/CMakeLists.txt

runners/bookmarks/browserfactory.cpp

runners/bookmarks/browsers/chrome.h

runners/bookmarks/browsers/chrome.cpp

runners/bookmarks/browsers/chromefindprofile.h

runners/bookmarks/browsers/chromefindprofile.cpp

runners/bookmarks/browsers/chromeprofile.h

runners/bookmarks/browsers/chromeprofile.cpp

runners/bookmarks/browsers/findprofile.h

runners/bookmarks/faviconfromblob.h

runners/bookmarks/faviconfromblob.cpp

runners/bookmarks/fetchsqlite.h

runners/bookmarks/fetchsqlite.cpp

runners/bookmarks/tests/CMakeLists.txt

runners/bookmarks/tests/chrome-config-home/.config/chromium/Local State

runners/bookmarks/tests/chrome-config-home/Chrome-Bookmarks-Sample.json

runners/bookmarks/tests/chrome-config-home/Chrome-Bookmarks-SecondProfile.json

runners/bookmarks/tests/chromium/Default/Bookmarks

runners/bookmarks/tests/chromium/Local State

runners/bookmarks/tests/chromium/Profile 1/Bookmarks

runners/bookmarks/tests/testchromebookmarks.h

runners/bookmarks/tests/testchromebookmarks.cpp

Change Chrome API designAcceptedPublicActions

Details

Diff Detail

Revision ContentsChangeset List

Diff 83118

runners/bookmarks/CMakeLists.txt

runners/bookmarks/browserfactory.cpp

runners/bookmarks/browsers/chrome.h

runners/bookmarks/browsers/chrome.cpp

runners/bookmarks/browsers/chromefindprofile.h

runners/bookmarks/browsers/chromefindprofile.cpp

runners/bookmarks/browsers/chromeprofile.h

runners/bookmarks/browsers/chromeprofile.cpp

runners/bookmarks/browsers/findprofile.h

runners/bookmarks/faviconfromblob.h

runners/bookmarks/faviconfromblob.cpp

runners/bookmarks/fetchsqlite.h

runners/bookmarks/fetchsqlite.cpp

runners/bookmarks/tests/CMakeLists.txt

runners/bookmarks/tests/chrome-config-home/.config/chromium/Local State

runners/bookmarks/tests/chrome-config-home/Chrome-Bookmarks-Sample.json

runners/bookmarks/tests/chrome-config-home/Chrome-Bookmarks-SecondProfile.json

runners/bookmarks/tests/chromium/Default/Bookmarks

runners/bookmarks/tests/chromium/Local State

runners/bookmarks/tests/chromium/Profile 1/Bookmarks

runners/bookmarks/tests/testchromebookmarks.h

runners/bookmarks/tests/testchromebookmarks.cpp

Change Chrome API design
AcceptedPublic
Actions

Revision Contents
Changeset List