Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2924383003:
[MD Bookmarks] Enable by default. (Closed)
Description[MD Bookmarks] Enable by default.
BUG=658980
Review-Url: https://codereview.chromium.org/2924383003
Cr-Commit-Position: refs/heads/master@{#481444}
Committed: https://chromium.googlesource.com/chromium/src/+/ae7fed4fc2b5d2e9732c4b4ed63ca2a639f05228
Patch Set 1 #Patch Set 2 : fix tests #Patch Set 3 : more tests #
Total comments: 10
Patch Set 4 : address comments #
Total comments: 8
Patch Set 5 : rebase, address comments #
Total comments: 7
Patch Set 6 : address comments #Messages
Total messages: 53 (34 generated)
calamity@chromium.org changed reviewers: + tsergeant@chromium.org
lgtm, but I wouldn't be too shocked if something in the test suite didn't like the old bookmark manager disappearing
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
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...
Want to have another look before I shard it off to OWNERS? I'm a bit iffy about the ComponentLoader test, but I don't think there are any other default component extensions that get overridden.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2924383003/diff/40001/chrome/browser/chrome_s... File chrome/browser/chrome_security_exploit_browsertest.cc (right): https://codereview.chromium.org/2924383003/diff/40001/chrome/browser/chrome_s... chrome/browser/chrome_security_exploit_browsertest.cc:96: // Target the bookmark manager extension. Should update this comment https://codereview.chromium.org/2924383003/diff/40001/chrome/browser/chrome_s... chrome/browser/chrome_security_exploit_browsertest.cc:145: std::string target_origin = "chrome-extension://" + extension()->id(); And here too https://codereview.chromium.org/2924383003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/component_loader_unittest.cc (right): https://codereview.chromium.org/2924383003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/component_loader_unittest.cc:262: EXPECT_EQ(default_count + 1, component_loader_.registered_extensions_count()); OWNERS might know more, but I think the idea of this test is not to test the chrome://bookmarks URL override (as in chrome/test/data/extensions/browsertest/url_rewrite/bookmarks/manifest.json), but instead to check that the component loader can replace a built-in component extension with a separate extension on the fly. To fix this test, I think you should change chrome/test/data/extensions/override_component_extension/manifest.json to mimic a different component extension like the PDF viewer, by copying over the "key" field from chrome/browser/resources/pdf/manifest.json into this file. https://codereview.chromium.org/2924383003/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc (right): https://codereview.chromium.org/2924383003/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:218: // (the bookmark manager is implemented as an extension) update this comment too https://codereview.chromium.org/2924383003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/bookmarks_ui_browsertest.cc (right): https://codereview.chromium.org/2924383003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/bookmarks_ui_browsertest.cc:59: if (MdBookmarksUI::IsEnabled()) Can these tests remain enabled for MD Bookmarks? They all seem to do things we still care about, and you would maybe just need to change AssertIsBookmarksPage() to get them working again.
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/2924383003/diff/40001/chrome/browser/chrome_s... File chrome/browser/chrome_security_exploit_browsertest.cc (right): https://codereview.chromium.org/2924383003/diff/40001/chrome/browser/chrome_s... chrome/browser/chrome_security_exploit_browsertest.cc:96: // Target the bookmark manager extension. On 2017/06/15 07:23:44, tsergeant wrote: > Should update this comment Done. https://codereview.chromium.org/2924383003/diff/40001/chrome/browser/chrome_s... chrome/browser/chrome_security_exploit_browsertest.cc:145: std::string target_origin = "chrome-extension://" + extension()->id(); On 2017/06/15 07:23:44, tsergeant wrote: > And here too Done. https://codereview.chromium.org/2924383003/diff/40001/chrome/browser/extensio... File chrome/browser/extensions/component_loader_unittest.cc (right): https://codereview.chromium.org/2924383003/diff/40001/chrome/browser/extensio... chrome/browser/extensions/component_loader_unittest.cc:262: EXPECT_EQ(default_count + 1, component_loader_.registered_extensions_count()); On 2017/06/15 07:23:44, tsergeant wrote: > OWNERS might know more, but I think the idea of this test is not to test the > chrome://bookmarks URL override (as in > chrome/test/data/extensions/browsertest/url_rewrite/bookmarks/manifest.json), > but instead to check that the component loader can replace a built-in component > extension with a separate extension on the fly. > > To fix this test, I think you should change > chrome/test/data/extensions/override_component_extension/manifest.json to mimic > a different component extension like the PDF viewer, by copying over the "key" > field from chrome/browser/resources/pdf/manifest.json into this file. Done. I wasn't aware of other overrideable component extensions. This is definitely better, and I think equivalent to the old test. https://codereview.chromium.org/2924383003/diff/40001/chrome/browser/renderer... File chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc (right): https://codereview.chromium.org/2924383003/diff/40001/chrome/browser/renderer... chrome/browser/renderer_host/render_process_host_chrome_browsertest.cc:218: // (the bookmark manager is implemented as an extension) On 2017/06/15 07:23:44, tsergeant wrote: > update this comment too Done. https://codereview.chromium.org/2924383003/diff/40001/chrome/browser/ui/webui... File chrome/browser/ui/webui/bookmarks_ui_browsertest.cc (right): https://codereview.chromium.org/2924383003/diff/40001/chrome/browser/ui/webui... chrome/browser/ui/webui/bookmarks_ui_browsertest.cc:59: if (MdBookmarksUI::IsEnabled()) On 2017/06/15 07:23:44, tsergeant wrote: > Can these tests remain enabled for MD Bookmarks? They all seem to do things we > still care about, and you would maybe just need to change > AssertIsBookmarksPage() to get them working again. Done. This test in particular has been deleted because it's a subset of BookmarksLoaded.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm
calamity@chromium.org changed reviewers: + alexmos@chromium.org, avi@chromium.org, benwells@chromium.org, sky@chromium.org
+sky@ for render_process_host_chrome_browsertest.cc +alexmos@ for chrome_security_exploit_browsertest.cc +avi@ for view_source_browsertest.cc +benwells@ for chrome/browser/extensions/ PTAL, thanks!
https://codereview.chromium.org/2924383003/diff/60001/chrome/browser/tab_cont... File chrome/browser/tab_contents/view_source_browsertest.cc (right): https://codereview.chromium.org/2924383003/diff/60001/chrome/browser/tab_cont... chrome/browser/tab_contents/view_source_browsertest.cc:192: IN_PROC_BROWSER_TEST_F(ViewSourceTest, ViewSourceTabRestore) { Can we move this test to chrome/browser/extensions? Seems like it belongs there.
render_process_host_chrome_browsertest.cc LGTM
https://codereview.chromium.org/2924383003/diff/60001/chrome/browser/chrome_s... File chrome/browser/chrome_security_exploit_browsertest.cc (right): https://codereview.chromium.org/2924383003/diff/60001/chrome/browser/chrome_s... chrome/browser/chrome_security_exploit_browsertest.cc:67: "/chrome_extension_resource.html"); Does this test also need to be fixed? chrome_extension_resource.html also hardcodes a few uses of the eemcgdkfndhakfknompkggombfjjjeno ID. https://codereview.chromium.org/2924383003/diff/60001/chrome/browser/chrome_s... chrome/browser/chrome_security_exploit_browsertest.cc:96: // Target the an extension. nit: s/the an/an/ https://codereview.chromium.org/2924383003/diff/60001/chrome/browser/chrome_s... chrome/browser/chrome_security_exploit_browsertest.cc:144: // Target the an extension. ditto
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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Patchset #5 (id:80001) 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/2924383003/diff/60001/chrome/browser/chrome_s... File chrome/browser/chrome_security_exploit_browsertest.cc (right): https://codereview.chromium.org/2924383003/diff/60001/chrome/browser/chrome_s... chrome/browser/chrome_security_exploit_browsertest.cc:67: "/chrome_extension_resource.html"); On 2017/06/20 20:26:02, alexmos wrote: > Does this test also need to be fixed? chrome_extension_resource.html also > hardcodes a few uses of the eemcgdkfndhakfknompkggombfjjjeno ID. Good point. Done. Switched extension to have an image to access. https://codereview.chromium.org/2924383003/diff/60001/chrome/browser/chrome_s... chrome/browser/chrome_security_exploit_browsertest.cc:96: // Target the an extension. On 2017/06/20 20:26:02, alexmos wrote: > nit: s/the an/an/ Done. https://codereview.chromium.org/2924383003/diff/60001/chrome/browser/chrome_s... chrome/browser/chrome_security_exploit_browsertest.cc:144: // Target the an extension. On 2017/06/20 20:26:02, alexmos wrote: > ditto Done. https://codereview.chromium.org/2924383003/diff/60001/chrome/browser/tab_cont... File chrome/browser/tab_contents/view_source_browsertest.cc (right): https://codereview.chromium.org/2924383003/diff/60001/chrome/browser/tab_cont... chrome/browser/tab_contents/view_source_browsertest.cc:192: IN_PROC_BROWSER_TEST_F(ViewSourceTest, ViewSourceTabRestore) { On 2017/06/20 02:20:38, Avi (ping after 24h) wrote: > Can we move this test to chrome/browser/extensions? Seems like it belongs there. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
chrome_security_exploit_browsertest.cc LGTM https://codereview.chromium.org/2924383003/diff/100001/chrome/test/data/chrom... File chrome/test/data/chrome_extension_resource.html (right): https://codereview.chromium.org/2924383003/diff/100001/chrome/test/data/chrom... chrome/test/data/chrome_extension_resource.html:5: var imgUrl = 'chrome-extension://ombfbiolafbecdcaofoilglhmobpdbnd/icon.png'; nit: maybe add a comment mentioning which extension this is? It's hard to tell looking only at this file.
lgtm https://codereview.chromium.org/2924383003/diff/100001/chrome/browser/tab_con... File chrome/browser/tab_contents/view_source_browsertest.cc (right): https://codereview.chromium.org/2924383003/diff/100001/chrome/browser/tab_con... chrome/browser/tab_contents/view_source_browsertest.cc:21: #include "extensions/common/constants.h" With the removal of the extensions test from this file, you might be able to drop some of these includes. Probably this one at least.
lgtm with a nit. https://codereview.chromium.org/2924383003/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/view_extension_source_browsertest.cc (right): https://codereview.chromium.org/2924383003/diff/100001/chrome/browser/extensi... chrome/browser/extensions/view_extension_source_browsertest.cc:12: #include "content/public/browser/navigation_entry.h" Nit: I don't think this include is needed.
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/2924383003/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/view_extension_source_browsertest.cc (right): https://codereview.chromium.org/2924383003/diff/100001/chrome/browser/extensi... chrome/browser/extensions/view_extension_source_browsertest.cc:12: #include "content/public/browser/navigation_entry.h" On 2017/06/22 01:55:20, benwells wrote: > Nit: I don't think this include is needed. This is GetActiveEntry()->GetVirtualURL(). https://codereview.chromium.org/2924383003/diff/100001/chrome/browser/tab_con... File chrome/browser/tab_contents/view_source_browsertest.cc (right): https://codereview.chromium.org/2924383003/diff/100001/chrome/browser/tab_con... chrome/browser/tab_contents/view_source_browsertest.cc:21: #include "extensions/common/constants.h" On 2017/06/21 19:24:04, Avi (ping after 24h) wrote: > With the removal of the extensions test from this file, you might be able to > drop some of these includes. Probably this one at least. Done. https://codereview.chromium.org/2924383003/diff/100001/chrome/test/data/chrom... File chrome/test/data/chrome_extension_resource.html (right): https://codereview.chromium.org/2924383003/diff/100001/chrome/test/data/chrom... chrome/test/data/chrome_extension_resource.html:5: var imgUrl = 'chrome-extension://ombfbiolafbecdcaofoilglhmobpdbnd/icon.png'; On 2017/06/21 18:09:01, alexmos wrote: > nit: maybe add a comment mentioning which extension this is? It's hard to tell > looking only at this file. Done.
https://codereview.chromium.org/2924383003/diff/100001/chrome/browser/extensi... File chrome/browser/extensions/view_extension_source_browsertest.cc (right): https://codereview.chromium.org/2924383003/diff/100001/chrome/browser/extensi... chrome/browser/extensions/view_extension_source_browsertest.cc:12: #include "content/public/browser/navigation_entry.h" On 2017/06/22 04:24:49, calamity wrote: > On 2017/06/22 01:55:20, benwells wrote: > > Nit: I don't think this include is needed. > > This is GetActiveEntry()->GetVirtualURL(). Acknowledged.
On 2017/06/22 04:24:50, calamity wrote: > https://codereview.chromium.org/2924383003/diff/100001/chrome/browser/extensi... > File chrome/browser/extensions/view_extension_source_browsertest.cc (right): > > https://codereview.chromium.org/2924383003/diff/100001/chrome/browser/extensi... > chrome/browser/extensions/view_extension_source_browsertest.cc:12: #include > "content/public/browser/navigation_entry.h" > On 2017/06/22 01:55:20, benwells wrote: > > Nit: I don't think this include is needed. > > This is GetActiveEntry()->GetVirtualURL(). > > https://codereview.chromium.org/2924383003/diff/100001/chrome/browser/tab_con... > File chrome/browser/tab_contents/view_source_browsertest.cc (right): > > https://codereview.chromium.org/2924383003/diff/100001/chrome/browser/tab_con... > chrome/browser/tab_contents/view_source_browsertest.cc:21: #include > "extensions/common/constants.h" > On 2017/06/21 19:24:04, Avi (ping after 24h) wrote: > > With the removal of the extensions test from this file, you might be able to > > drop some of these includes. Probably this one at least. > > Done. > > https://codereview.chromium.org/2924383003/diff/100001/chrome/test/data/chrom... > File chrome/test/data/chrome_extension_resource.html (right): > > https://codereview.chromium.org/2924383003/diff/100001/chrome/test/data/chrom... > chrome/test/data/chrome_extension_resource.html:5: var imgUrl = > 'chrome-extension://ombfbiolafbecdcaofoilglhmobpdbnd/icon.png'; > On 2017/06/21 18:09:01, alexmos wrote: > > nit: maybe add a comment mentioning which extension this is? It's hard to > tell > > looking only at this file. > > Done. Still LGTM and good luck.
The CQ bit was unchecked by calamity@chromium.org
The CQ bit was checked by calamity@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tsergeant@chromium.org, sky@chromium.org, benwells@chromium.org, alexmos@chromium.org Link to the patchset: https://codereview.chromium.org/2924383003/#ps120001 (title: "address comments")
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": 120001, "attempt_start_ts": 1498106949468180,
"parent_rev": "02958eaa8c765b0bc65c020bb1dd1d1fd0792de0", "commit_rev":
"ae7fed4fc2b5d2e9732c4b4ed63ca2a639f05228"}
Message was sent while issue was closed.
Description was changed from ========== [MD Bookmarks] Enable by default. BUG=658980 ========== to ========== [MD Bookmarks] Enable by default. BUG=658980 Review-Url: https://codereview.chromium.org/2924383003 Cr-Commit-Position: refs/heads/master@{#481444} Committed: https://chromium.googlesource.com/chromium/src/+/ae7fed4fc2b5d2e9732c4b4ed63c... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/ae7fed4fc2b5d2e9732c4b4ed63c... |
