Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Get back Tabs from other devices history section; #7121

Merged
merged 3 commits into from
Nov 30, 2020
Merged

Get back Tabs from other devices history section; #7121

merged 3 commits into from
Nov 30, 2020

Conversation

AlexeyBarabash
Copy link
Contributor

@AlexeyBarabash AlexeyBarabash commented Nov 11, 2020 •
edited by bsclifton
Loading

This reverts some changes from #212 and adds More... item into History menu for devices which have tabs number in session more than 4.

Resolves brave/brave-browser#10473
Resolves brave/brave-browser#11183

Submitter Checklist:

Test Plan:

  1. Create sync chain with 2 devices (A and B)
  2. Enable on devices History and Open Tabs sync types
  3. On device A open 3 tabs, excluding internal pages (brave://****), like: google.com, apple.com, microsoft.com
  4. On device B open 5 tabs, excluding internal pages (brave://****), like: cnn.com, espn.com, nytimes.com, reddit.com, ebay.com
  5. On devices A and B open History page (Ctrl+H or brave://history) ensure there is a section Tabs from other devices (you may need to extend width if the width of a window is small)
  6. On device A open app menu, then History sub menu, ensure you can see tabs from device B and More... item which leads to brave://history/syncedTabs
  7. On device B open app menu, then History sub menu, ensure you can see 3 tabs from device A and there is no More... item

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@AlexeyBarabash AlexeyBarabash self-assigned this Nov 11, 2020
@AlexeyBarabash
Copy link
Contributor Author

For now it looks
Screenshot from 2020-11-11 19-02-21

Need to make an additional menu item in main menu => History => Device

@AlexeyBarabash AlexeyBarabash force-pushed the history_other_devices branch 3 times, most recently from dde2d41 to 8b7ffec Compare November 17, 2020 14:51
@AlexeyBarabash
Copy link
Contributor Author

The screenshot of the extended menu, note that session from phone mi4c has only 2 entries, so it doesn't have More... item:
Screenshot from 2020-11-17 20-22-28_2

@AlexeyBarabash
Copy link
Contributor Author

Faced with DCHECK and CHECK:

https://source.chromium.org/chromium/chromium/src/+/master:components/sync/model_impl/processor_entity.cc;l=181;drc=f50df03b5c021e63d80ecf09ea452fa9976071ea;bpv=1;bpt=1?q=ProcessorEntity::RecordIgnoredUpdate&ss=chromium%2Fchromium%2Fsrc

void ProcessorEntity::RecordIgnoredUpdate(const UpdateResponseData& update) {
  DCHECK(metadata_.server_id().empty() ||
         metadata_.server_id() == update.entity.id);

https://source.chromium.org/chromium/chromium/src/+/master:content/public/browser/web_ui_message_handler.h;l=125;drc=f50df03b5c021e63d80ecf09ea452fa9976071ea;bpv=1;bpt=1?q=content%2Fpublic%2Fbrowser%2Fweb_ui_message_handler.h&ss=chromium%2Fchromium%2Fsrc

  template <typename... Values>
  void CallJavascriptFunction(const std::string& function_name,
                              const Values&... values) {
    CHECK(IsJavascriptAllowed()) << "Cannot CallJavascriptFunction before "
                                    "explicitly allowing JavaScript.";

Looking into this before publish the PR.

@AlexeyBarabash AlexeyBarabash mentioned this pull request Nov 18, 2020
Closed
@AlexeyBarabash AlexeyBarabash force-pushed the history_other_devices branch 3 times, most recently from 517588e to da46567 Compare November 19, 2020 22:00
@AlexeyBarabash AlexeyBarabash marked this pull request as ready for review November 19, 2020 22:24
@AlexeyBarabash AlexeyBarabash requested a review from a team as a code owner November 19, 2020 22:24
@AlexeyBarabash
Copy link
Contributor Author

I wasn't able to see the DCHECK and CHECK mentioned in #7121 (comment), so opened the PR

This was referenced Nov 20, 2020
Closed
Closed
petemill
petemill requested changes Nov 20, 2020
View reviewed changes
Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor string change

app/brave_generated_resources.grd Outdated
@@ -172,6 +172,9 @@
<message name="IDS_OPEN_GUEST_PROFILE" desc="The app menu item to create a new User Profile">
Open Guest Window
</message>
<message name="IDS_OPEN_MORE_OTHER_DEVICES_SESSIONS" desc="The history sub-menu item to show sessions from other devices">
More...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use the ellipsis character …

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@petemill this is fixed at https://github.com/brave/brave-core/pull/7121/files#diff-6314ab9f5910c0e3dd53f8ac6bf09b584020abe6790abe7ebcdacc5756c5d5f9R176

chromium_src/chrome/browser/ui/toolbar/recent_tabs_sub_menu_model.h Outdated
private: \
std::auto_ptr<sessions::SessionTab> stub_tab_; \
public:
// define BRAVE_RECENT_TABS_SUB_MENU_MODEL_H_
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@petemill fixed at https://github.com/brave/brave-core/pull/7121/files#diff-a996d007564a56862f193109f092f542f325c0356535e1a4b42f11e00d7143f6R13

@AlexeyBarabash AlexeyBarabash marked this pull request as draft November 23, 2020 14:28
@AlexeyBarabash AlexeyBarabash marked this pull request as ready for review November 23, 2020 17:39
@AlexeyBarabash
Copy link
Contributor Author

macOS, iOS, Windows CI were aborted without any PR-specific error - restarted them all

@AlexeyBarabash
Copy link
Contributor Author

iOS node is disabled, so can't restart.

petemill
petemill approved these changes Nov 25, 2020
View reviewed changes
Copy link
Member

@petemill petemill left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and works well 👍 Great to see this working 🥳

mkarolin
mkarolin reviewed Nov 30, 2020
View reviewed changes
patches/chrome-VERSION.patch
@@ -1,5 +1,5 @@
diff --git a/chrome/VERSION b/chrome/VERSION
index 2c4514a701725958eed9feacc7a43c46ef1f2fba..205283ea59b247e3aa5ac69d0d81f2c05133c5b8 100644
index 2c4514a701725958eed9feacc7a43c46ef1f2fba..6d423561e1ebcc4ca78cb414346bbb09695b328f 100644
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This patch shouldn't be in the PR. No need to rerun CI if you remove it.

mkarolin
mkarolin approved these changes Nov 30, 2020
View reviewed changes
Copy link
Collaborator

@mkarolin mkarolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chromium_src/patches changes LGTM

@AlexeyBarabash AlexeyBarabash merged commit 9b36eb8 into master Nov 30, 2020
@AlexeyBarabash AlexeyBarabash deleted the history_other_devices branch November 30, 2020 21:55
@AlexeyBarabash
Copy link
Contributor Author

Verified it works on Nightly.

Brave 1.19.43 Chromium: 87.0.4280.67 (Official Build) nightly (64-bit)
Revision 0e5d92df40086cf0050c00f87b11da1b14580930-refs/branch-heads/4280@{#1441}

@bsclifton bsclifton added this to the 1.19.x - Nightly milestone Dec 1, 2020
@bsclifton
Copy link
Member

Set missing milestone

@GeetaSarvadnya GeetaSarvadnya mentioned this pull request Dec 24, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkarolin mkarolin mkarolin approved these changes

@petemill petemill petemill approved these changes

@bbondy bbondy Awaiting requested review from bbondy

Assignees

@AlexeyBarabash AlexeyBarabash

Labels
enhancement feature/sync UI
Projects
None yet
Milestone
1.19.x - Release
4 participants
@AlexeyBarabash @bsclifton @petemill @mkarolin