Change Chrome API design
AcceptedPublic

Authored by alex on May 17 2020, 4:14 PM.

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 27010
Build 27028: 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
anthonyfieroni added a subscriber: anthonyfieroni.

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.

alex updated this revision to Diff 83028.May 17 2020, 6:08 PM

Make constructor private

alex updated this revision to Diff 83029.May 17 2020, 8:10 PM
alex marked an inline comment as done.

Create test cases

alex updated this revision to Diff 83116.May 22 2020, 3:35 PM
alex retitled this revision from WIP: Change Chrome API design to Change Chrome API design.
alex edited the summary of this revision. (Show Details)
alex added reviewers: ngraham, meven.

Change import, formatting

meven added a comment.May 22 2020, 4:56 PM

Seems good, two small clean ups

runners/bookmarks/browsers/chromeprofile.cpp
9

{ on new line

28

Remove

alex updated this revision to Diff 83118.May 22 2020, 5:02 PM

Formatting, remove debug statement

alex marked 2 inline comments as done.May 22 2020, 5:02 PM
meven accepted this revision as: meven.May 23 2020, 2:04 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
34

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
alex updated this revision to Diff 83131.May 23 2020, 3:56 PM

Use iterator, remove unused include

alex marked 2 inline comments as done.May 23 2020, 3:57 PM
meven accepted this revision.Jun 3 2020, 7:24 AM

@broulik perhaps a review ?

alex added a comment.Jun 11 2020, 9:42 AM

Friendly Ping :-)

Or should I just move this to gitlab?

meven added a comment.Jun 16 2020, 1:42 PM
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