Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2899223002:
[MD Bookmarks] Fix singleton tab behavior. (Closed)
Description[MD Bookmarks] Fix singleton tab behavior.
This CL fixes an issue where MD Bookmarks would not focus an existing
tab when the bookmark manager was launched a second time. This was
happening because the query of the URL was being used in the deduping
process, so calling a function which sets IGNORE_AND_NAVIGATE fixes this.
It also fixes an issue where repeatedly pressing the bookmark manager
shortcut would repeatedly refresh the page because the specified URL
is chrome://bookmarks but the redirected URL is chrome://bookmarks/?id=1.
BUG=725761
Review-Url: https://codereview.chromium.org/2899223002
Cr-Commit-Position: refs/heads/master@{#475290}
Committed: https://chromium.googlesource.com/chromium/src/+/870ec553a786c0f19d6785a4333fa7a6dc2885c0
Patch Set 1 #
Total comments: 4
Patch Set 2 : address_comments #Patch Set 3 : add bookmark node constants #
Total comments: 2
Patch Set 4 : address comments #
Total comments: 2
Patch Set 5 : mark histogram obsolete #
Messages
Total messages: 33 (22 generated)
Description was changed from ========== [MD Bookmarks] Fix singleton tab behavior. This CL fixes an issue where MD Bookmarks would not focus an existing tab when the bookmark manager was launched a second time. This was happening because the query of the URL was being used in the deduping process, so setting IGNORE_AND_NAVIGATE fixes this. It also fixes an issue where repeatedly pressing the bookmark manager shortcut would repeatedly refresh the page because the specified URL is chrome://bookmarks but the redirected URL is chrome://bookmarks/?id=1. BUG=725761 ========== to ========== [MD Bookmarks] Fix singleton tab behavior. This CL fixes an issue where MD Bookmarks would not focus an existing tab when the bookmark manager was launched a second time. This was happening because the query of the URL was being used in the deduping process, so calling a function which sets IGNORE_AND_NAVIGATE fixes this. It also fixes an issue where repeatedly pressing the bookmark manager shortcut would repeatedly refresh the page because the specified URL is chrome://bookmarks but the redirected URL is chrome://bookmarks/?id=1. BUG=725761 ==========
calamity@chromium.org changed reviewers: + sky@chromium.org
+sky for chrome/browser/ui/ OWNERS. PTAL, thanks!
https://codereview.chromium.org/2899223002/diff/1/chrome/browser/ui/chrome_pa... File chrome/browser/ui/chrome_pages.cc (right): https://codereview.chromium.org/2899223002/diff/1/chrome/browser/ui/chrome_pa... chrome/browser/ui/chrome_pages.cc:188: OpenBookmarkManagerForNode(browser, 1); Where does the 1 come from here? https://codereview.chromium.org/2899223002/diff/1/chrome/browser/ui/chrome_pa... chrome/browser/ui/chrome_pages.cc:199: base::RecordAction(UserMetricsAction("ShowBookmarks")); Why do we have two user-metrics-actions for this?
The CQ bit was checked by calamity@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Uploaded a patchset with and without constants added to bookmark_model.h. https://codereview.chromium.org/2899223002/diff/1/chrome/browser/ui/chrome_pa... File chrome/browser/ui/chrome_pages.cc (right): https://codereview.chromium.org/2899223002/diff/1/chrome/browser/ui/chrome_pa... chrome/browser/ui/chrome_pages.cc:188: OpenBookmarkManagerForNode(browser, 1); On 2017/05/24 17:16:06, sky wrote: > Where does the 1 come from here? It's the id for the bookmarks bar. This isn't actually defined in a constant anywhere, but is guaranteed by the way the bookmarks model is constructed and assumed by the old bookmark manager. https://cs.chromium.org/chromium/src/components/bookmarks/browser/bookmark_mo... creates the root nodes in order which uses generate_next_node_id() which starts from 1. The old bookmark manager hardcodes these in JS here https://cs.chromium.org/chromium/src/chrome/browser/resources/bookmark_manage... I've altered this to be a little more obvious about what it's doing, but maybe it's worth adding these root folder ids as constants inside bookmark_model.h. WDYT? https://codereview.chromium.org/2899223002/diff/1/chrome/browser/ui/chrome_pa... chrome/browser/ui/chrome_pages.cc:199: base::RecordAction(UserMetricsAction("ShowBookmarks")); On 2017/05/24 17:16:06, sky wrote: > Why do we have two user-metrics-actions for this? Good point. I audited and these are used for the same thing, but the calls are still all wrong and messed up. Fixed it so that these two places are the only places it gets logged.
The CQ bit was checked by calamity@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:60001) has been deleted
https://codereview.chromium.org/2899223002/diff/40001/components/bookmarks/br... File components/bookmarks/browser/bookmark_model.cc (right): https://codereview.chromium.org/2899223002/diff/40001/components/bookmarks/br... components/bookmarks/browser/bookmark_model.cc:113: const int64_t BookmarkModel::BOOKMARKS_BAR_NODE_ID = 1; Rather than exposing these, how about using bookmark_bar_node()->id()?
Patchset #4 (id:80001) has been deleted
Patchset #4 (id:100001) has been deleted
The CQ bit was checked by calamity@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2899223002/diff/40001/components/bookmarks/br... File components/bookmarks/browser/bookmark_model.cc (right): https://codereview.chromium.org/2899223002/diff/40001/components/bookmarks/br... components/bookmarks/browser/bookmark_model.cc:113: const int64_t BookmarkModel::BOOKMARKS_BAR_NODE_ID = 1; On 2017/05/25 18:07:05, sky wrote: > Rather than exposing these, how about using bookmark_bar_node()->id()? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
calamity@chromium.org changed reviewers: + asvitkine@chromium.org
+asvitkine for actions.xml
lgtm % comment https://codereview.chromium.org/2899223002/diff/120001/tools/metrics/actions/... File tools/metrics/actions/actions.xml (left): https://codereview.chromium.org/2899223002/diff/120001/tools/metrics/actions/... tools/metrics/actions/actions.xml:14812: <action name="ShowBookmarks"> Please mark it as <obsolete> instead since it's used to decode data from old versions.
The CQ bit was checked by calamity@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2899223002/diff/120001/tools/metrics/actions/... File tools/metrics/actions/actions.xml (left): https://codereview.chromium.org/2899223002/diff/120001/tools/metrics/actions/... tools/metrics/actions/actions.xml:14812: <action name="ShowBookmarks"> On 2017/05/26 14:55:38, Alexei Svitkine (slow) wrote: > Please mark it as <obsolete> instead since it's used to decode data from old > versions. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by calamity@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2899223002/#ps140001 (title: "mark histogram obsolete")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1496034564016200,
"parent_rev": "9453f30a545c2125da8fc2aac5aa9b329c6fe438", "commit_rev":
"870ec553a786c0f19d6785a4333fa7a6dc2885c0"}
Message was sent while issue was closed.
Description was changed from ========== [MD Bookmarks] Fix singleton tab behavior. This CL fixes an issue where MD Bookmarks would not focus an existing tab when the bookmark manager was launched a second time. This was happening because the query of the URL was being used in the deduping process, so calling a function which sets IGNORE_AND_NAVIGATE fixes this. It also fixes an issue where repeatedly pressing the bookmark manager shortcut would repeatedly refresh the page because the specified URL is chrome://bookmarks but the redirected URL is chrome://bookmarks/?id=1. BUG=725761 ========== to ========== [MD Bookmarks] Fix singleton tab behavior. This CL fixes an issue where MD Bookmarks would not focus an existing tab when the bookmark manager was launched a second time. This was happening because the query of the URL was being used in the deduping process, so calling a function which sets IGNORE_AND_NAVIGATE fixes this. It also fixes an issue where repeatedly pressing the bookmark manager shortcut would repeatedly refresh the page because the specified URL is chrome://bookmarks but the redirected URL is chrome://bookmarks/?id=1. BUG=725761 Review-Url: https://codereview.chromium.org/2899223002 Cr-Commit-Position: refs/heads/master@{#475290} Committed: https://chromium.googlesource.com/chromium/src/+/870ec553a786c0f19d6785a4333f... ==========
Message was sent while issue was closed.
Committed patchset #5 (id:140001) as https://chromium.googlesource.com/chromium/src/+/870ec553a786c0f19d6785a4333f... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
