Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(27)

Issue 1414643005: Disable zoom bubble for PDF extension. (Closed)

Created:
5 years, 2 months ago by wjmaclean
Modified:
5 years, 1 month ago
CC:
chromium-apps-reviews_chromium.org, chromium-reviews, extensions-reviews_chromium.org, tfarina
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Disable zoom bubble for PDF extension. The location-bar zoom bubble is interfering with the material-design top-toolbar in the PDF viewer. Since the viewer is a trusted extension, we can disable the bubble for it. BUG=538252 Committed: https://crrev.com/70eb6d831495b034bc6c2ed259de428f933a8386 Cr-Commit-Position: refs/heads/master@{#356946}

Patch Set 1 #

Patch Set 2 : Need to check for valid client before early out. #

Total comments: 4

Patch Set 3 : Improve function name, add bug link. #

Patch Set 4 : Add test. #

Patch Set 5 : Fix GN dependency. #

Patch Set 6 : New test doesn't run on Mac. #

Patch Set 7 : Convert to pathway common to Mac and non-Mac. #

Total comments: 18

Patch Set 8 : Rebase, address suggestions. #

Patch Set 9 : git add deps file #

Total comments: 1

Patch Set 10 : Exclude ZoomBubbleView references from Mac version of tests. #

Patch Set 11 : Fix rebase. #

Patch Set 12 : Test rebuilding patch from .diff file. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+202 lines, -37 lines) Patch
A chrome/browser/pdf/DEPS View 1 2 3 4 5 6 7 1 chunk +4 lines, -0 lines 0 comments Download
M chrome/browser/pdf/pdf_extension_test.cc View 1 2 3 4 5 6 7 8 9 4 chunks +52 lines, -0 lines 0 comments Download
M chrome/browser/ui/zoom/zoom_controller_unittest.cc View 1 2 3 4 5 6 7 8 9 2 chunks +2 lines, -37 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M chrome/chrome_tests_unit.gypi View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M components/ui/zoom/BUILD.gn View 1 2 3 4 5 6 7 8 9 1 chunk +13 lines, -0 lines 0 comments Download
A + components/ui/zoom/test/DEPS View 1 2 3 4 5 6 7 8 0 chunks +-1 lines, --1 lines 0 comments Download
A components/ui/zoom/test/zoom_test_utils.h View 1 2 3 4 5 6 7 8 9 1 chunk +42 lines, -0 lines 0 comments Download
A components/ui/zoom/test/zoom_test_utils.cc View 1 2 3 4 5 6 7 8 9 1 chunk +43 lines, -0 lines 0 comments Download
M components/ui/zoom/zoom_controller.h View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M components/ui/zoom/zoom_controller.cc View 1 2 3 4 5 6 1 chunk +5 lines, -1 line 0 comments Download
M components/ui_zoom.gypi View 1 2 3 4 5 6 7 8 9 1 chunk +17 lines, -0 lines 0 comments Download
M extensions/browser/extension_zoom_request_client.h View 1 2 3 4 5 6 1 chunk +1 line, -0 lines 0 comments Download
M extensions/browser/extension_zoom_request_client.cc View 1 2 3 4 5 6 2 chunks +10 lines, -0 lines 0 comments Download
M extensions/common/api/_behavior_features.json View 1 2 8 9 1 chunk +7 lines, -0 lines 0 comments Download
M extensions/common/features/behavior_feature.h View 8 9 1 chunk +1 line, -0 lines 0 comments Download
M extensions/common/features/behavior_feature.cc View 8 9 1 chunk +2 lines, -0 lines 0 comments Download

Depends on Patchset:

Messages

Total messages: 77 (33 generated)
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414643005/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414643005/1
5 years, 2 months ago (2015-10-20 19:09:59 UTC) #4
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/119602)
5 years, 2 months ago (2015-10-20 19:49:32 UTC) #6
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414643005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414643005/20001
5 years, 2 months ago (2015-10-21 14:55:24 UTC) #9
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 2 months ago (2015-10-21 15:36:50 UTC) #11
wjmaclean
Smallish-cl ... I think this will do what we need. Please take a look?
5 years, 2 months ago (2015-10-21 15:45:07 UTC) #12
Ken Rockot(use gerrit already)
lgtm, thanks! https://codereview.chromium.org/1414643005/diff/20001/chrome/browser/extensions/extension_util.cc File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/1414643005/diff/20001/chrome/browser/extensions/extension_util.cc#newcode201 chrome/browser/extensions/extension_util.cc:201: bool ZoomWithoutBubble(const Extension* extension) { nit: How ...
5 years, 2 months ago (2015-10-21 16:53:31 UTC) #13
wjmaclean
https://codereview.chromium.org/1414643005/diff/20001/chrome/browser/extensions/extension_util.cc File chrome/browser/extensions/extension_util.cc (right): https://codereview.chromium.org/1414643005/diff/20001/chrome/browser/extensions/extension_util.cc#newcode201 chrome/browser/extensions/extension_util.cc:201: bool ZoomWithoutBubble(const Extension* extension) { On 2015/10/21 16:53:30, Ken ...
5 years, 2 months ago (2015-10-21 16:59:47 UTC) #14
Lei Zhang
lgtm
5 years, 2 months ago (2015-10-21 17:05:12 UTC) #15
Lei Zhang
Anyway, we can add an automated test?
5 years, 2 months ago (2015-10-21 17:05:26 UTC) #16
wjmaclean
On 2015/10/21 17:05:26, Lei Zhang wrote: > Anyway, we can add an automated test? I'll ...
5 years, 2 months ago (2015-10-21 17:32:56 UTC) #17
wjmaclean
jochen@ - please review changes to components/ui_zoom.gypi thestig@ - please take another look; pdf_extension_test.cc and ...
5 years, 2 months ago (2015-10-23 13:21:07 UTC) #19
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414643005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414643005/60001
5 years, 2 months ago (2015-10-23 13:22:02 UTC) #21
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_compile_dbg_ng/builds/4628) linux_chromium_gn_chromeos_rel on ...
5 years, 2 months ago (2015-10-23 13:24:49 UTC) #23
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414643005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414643005/80001
5 years, 2 months ago (2015-10-23 14:24:40 UTC) #25
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/112189)
5 years, 2 months ago (2015-10-23 14:32:05 UTC) #27
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414643005/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414643005/100001
5 years, 2 months ago (2015-10-23 14:45:33 UTC) #29
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/112196)
5 years, 2 months ago (2015-10-23 14:53:13 UTC) #31
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414643005/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414643005/120001
5 years, 2 months ago (2015-10-23 17:11:37 UTC) #33
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator_ninja/builds/85354) ios_rel_device_ninja on ...
5 years, 2 months ago (2015-10-23 17:14:01 UTC) #36
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414643005/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414643005/140001
5 years, 2 months ago (2015-10-23 17:14:09 UTC) #37
wjmaclean
I realized the previous approach involved a pathway that had no counterpart on Mac, so ...
5 years, 2 months ago (2015-10-23 17:16:09 UTC) #38
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_compile_dbg_ng/builds/114094) mac_chromium_gn_rel on ...
5 years, 2 months ago (2015-10-23 17:16:34 UTC) #40
Ken Rockot(use gerrit already)
cool, lgtm https://codereview.chromium.org/1414643005/diff/140001/chrome/browser/pdf/pdf_extension_test.cc File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/1414643005/diff/140001/chrome/browser/pdf/pdf_extension_test.cc#newcode462 chrome/browser/pdf/pdf_extension_test.cc:462: EXPECT_EQ(nullptr, ZoomBubbleView::GetZoomBubble()); Sorry, I don't know much ...
5 years, 2 months ago (2015-10-23 17:58:43 UTC) #41
Ken Rockot(use gerrit already)
On 2015/10/23 17:58:43, Ken Rockot wrote: > cool, lgtm > > https://codereview.chromium.org/1414643005/diff/140001/chrome/browser/pdf/pdf_extension_test.cc > File chrome/browser/pdf/pdf_extension_test.cc ...
5 years, 2 months ago (2015-10-23 17:59:00 UTC) #42
Lei Zhang
mostly nitty: https://codereview.chromium.org/1414643005/diff/140001/chrome/browser/DEPS File chrome/browser/DEPS (right): https://codereview.chromium.org/1414643005/diff/140001/chrome/browser/DEPS#newcode180 chrome/browser/DEPS:180: # The next line is needed for ...
5 years, 2 months ago (2015-10-23 18:14:32 UTC) #43
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414643005/60002 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414643005/60002
5 years, 2 months ago (2015-10-23 22:32:34 UTC) #45
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ninja/builds/127542)
5 years, 2 months ago (2015-10-23 22:38:00 UTC) #47
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414643005/170001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414643005/170001
5 years, 2 months ago (2015-10-24 01:05:32 UTC) #49
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presubmit/builds/112450)
5 years, 2 months ago (2015-10-24 01:10:15 UTC) #51
jochen (gone - plz use gerrit)
lgtm
5 years, 1 month ago (2015-10-26 15:42:14 UTC) #52
wjmaclean
Sorry for the hiatus on this CL; I was away at an off-site for the ...
5 years, 1 month ago (2015-10-29 15:33:36 UTC) #53
Lei Zhang
https://codereview.chromium.org/1414643005/diff/170001/chrome/browser/pdf/pdf_extension_test.cc File chrome/browser/pdf/pdf_extension_test.cc (right): https://codereview.chromium.org/1414643005/diff/170001/chrome/browser/pdf/pdf_extension_test.cc#newcode50 chrome/browser/pdf/pdf_extension_test.cc:50: #if defined(TOOLKIT_VIEWS) I'd do this for now: #if defined(TOOLKIT_VIEWS) ...
5 years, 1 month ago (2015-10-29 16:40:33 UTC) #54
Lei Zhang
Otherwise lgtm++
5 years, 1 month ago (2015-10-29 16:41:16 UTC) #55
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414643005/190001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414643005/190001
5 years, 1 month ago (2015-10-29 17:58:45 UTC) #57
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator_ninja/builds/87698) ios_rel_device_ninja on ...
5 years, 1 month ago (2015-10-29 18:02:11 UTC) #59
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414643005/210001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414643005/210001
5 years, 1 month ago (2015-10-29 18:40:06 UTC) #61
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_compile_dbg_ng/builds/116479) mac_chromium_gn_rel on ...
5 years, 1 month ago (2015-10-29 18:42:40 UTC) #63
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414643005/230001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414643005/230001
5 years, 1 month ago (2015-10-29 18:50:17 UTC) #66
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_compile_dbg_ng/builds/116491) mac_chromium_gn_rel on ...
5 years, 1 month ago (2015-10-29 18:52:49 UTC) #68
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414643005/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414643005/250001
5 years, 1 month ago (2015-10-29 19:38:38 UTC) #70
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years, 1 month ago (2015-10-29 20:53:56 UTC) #72
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1414643005/250001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1414643005/250001
5 years, 1 month ago (2015-10-29 21:02:07 UTC) #75
commit-bot: I haz the power
Committed patchset #12 (id:250001)
5 years, 1 month ago (2015-10-29 21:10:17 UTC) #76
commit-bot: I haz the power
5 years, 1 month ago (2015-10-29 21:11:56 UTC) #77
Message was sent while issue was closed.
Patchset 12 (id:??) landed as
https://crrev.com/70eb6d831495b034bc6c2ed259de428f933a8386
Cr-Commit-Position: refs/heads/master@{#356946}

Powered by Google App Engine
This is Rietveld 408576698