Show a custom message in the page info bubble for view-source URLs

Show a custom message in the page info bubble for view-source URLs

Screenshot: https://drive.google.com/file/d/0B9q2eN9gDoUIMGYyeE85Ynk2VGs/view?usp=sharing

BUG=635776
TBR=sdefresne,caitkp

Committed: https://crrev.com/e1feca06c43dba6d9465e9a06370cbab24f5d7fe
Cr-Commit-Position: refs/heads/master@{#412067}

[meacer, 2016-08-09 18:14:15]

The CQ bit was checked by meacer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-09 18:14:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-09 18:47:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-09 18:47:31]

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...)

[meacer, 2016-08-10 17:45:50]

The CQ bit was checked by meacer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 17:46:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 18:10:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 18:10:00]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[meacer, 2016-08-10 19:07:36]

The CQ bit was checked by meacer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 19:08:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[meacer, 2016-08-10 19:08:35]

The CQ bit was checked by meacer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 19:08:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 19:46:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 19:46:49]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[meacer, 2016-08-10 19:50:59]

The CQ bit was checked by meacer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 19:51:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 20:15:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 20:15:18]

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...)

[meacer, 2016-08-10 20:17:13]

The CQ bit was checked by meacer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-10 20:17:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-10 20:47:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-10 20:47:46]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[meacer, 2016-08-11 22:42:49]

The CQ bit was checked by meacer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-11 22:44:01]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-11 23:58:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-11 23:58:26]

Dry run: This issue passed the CQ dry run.

[meacer, 2016-08-12 01:02:28]

Description was changed from

==========
Show a custom message in the page info bubble for view-source URLs

BUG=635776
==========

to

==========
Show a custom message in the page info bubble for view-source URLs

Screenshot:
https://drive.google.com/file/d/0B9q2eN9gDoUIMGYyeE85Ynk2VGs/view?usp=sharing


BUG=635776
==========

[meacer, 2016-08-12 01:04:04]

meacer@chromium.org changed reviewers:
+ felt@chromium.org

[meacer, 2016-08-12 01:04:04]

felt for chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
rsesek for chrome/browser/ui/cocoa/website_settings/
sdefresne for components/

PTAL, thanks.

[meacer, 2016-08-12 01:04:23]

meacer@chromium.org changed reviewers:
+ rsesek@chromium.org, sdefresne@chromium.org

[meacer, 2016-08-12 01:05:35]

https://codereview.chromium.org/2232453002/diff/100001/components/pageinfo_st...
File components/pageinfo_strings.grdp (right):

https://codereview.chromium.org/2232453002/diff/100001/components/pageinfo_st...
components/pageinfo_strings.grdp:64: You are viewing source of a web page.
Should this say "the source of a web page", "sources of a web page", or else?

[felt, 2016-08-12 01:44:07]

felt@chromium.org changed reviewers:
+ lgarron@chromium.org

[felt, 2016-08-12 01:44:43]

lgarron, can you please do a shadow review? (i'll review tomorrow afternoon, if
you can take a look before then)

[meacer, 2016-08-12 17:16:58]

Note that this is a stable release blocker (not pushing it, just helping you
prioritize :)

[Robert Sesek, 2016-08-12 17:26:18]

LGTM

https://codereview.chromium.org/2232453002/diff/100001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm
(right):

https://codereview.chromium.org/2232453002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm:1163:
if (!virtual_url.SchemeIs(content::kChromeUIScheme) &&
!IsInternalURL() ?

[meacer, 2016-08-12 17:31:20]

The CQ bit was checked by meacer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-12 17:31:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-12 18:40:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-12 18:40:39]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[felt, 2016-08-12 18:58:39]

https://codereview.chromium.org/2232453002/diff/120001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm
(right):

https://codereview.chromium.org/2232453002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm:262:
}
If I'm reading this right, the default is the content::kChromeUIScheme because
this can only be reached for one of those three schemes. However it is not
explicitly called out here. I'm worried that in a future CL someone will add a
new scheme to IsInternalURL, and kChromeUIScheme will get the wrong treatment.

Would it be possible to keep the concept of a BubbleType so that you can have
switch statements, making sure all the BubbleTypes are handled?

At the very least, please be explicit about the kChromeUIScheme here.

[meacer, 2016-08-12 21:13:49]

https://codereview.chromium.org/2232453002/diff/100001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm
(right):

https://codereview.chromium.org/2232453002/diff/100001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm:1163:
if (!virtual_url.SchemeIs(content::kChromeUIScheme) &&
On 2016/08/12 17:26:18, Robert Sesek wrote:
> !IsInternalURL() ?

Done.

https://codereview.chromium.org/2232453002/diff/120001/chrome/browser/ui/coco...
File
chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm
(right):

https://codereview.chromium.org/2232453002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/website_settings/website_settings_bubble_controller.mm:262:
}
On 2016/08/12 18:58:39, felt wrote:
> If I'm reading this right, the default is the content::kChromeUIScheme because
> this can only be reached for one of those three schemes. However it is not
> explicitly called out here. I'm worried that in a future CL someone will add a
> new scheme to IsInternalURL, and kChromeUIScheme will get the wrong treatment.
> 
> Would it be possible to keep the concept of a BubbleType so that you can have
> switch statements, making sure all the BubbleTypes are handled?
> 
> At the very least, please be explicit about the kChromeUIScheme here.

I removed BubbleType for two reasons:
- Corresponding views code uses URLs, so this is more consistent
- BubbleType is only used here, and it seemed overkill to add an enum for only
this reason. (it was me who added it in a previous CL)

Hopefully it's clearer now with the NOTREACHED.

[felt, 2016-08-12 23:58:43]

https://codereview.chromium.org/2232453002/diff/140001/chrome/browser/ui/view...
File chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
(right):

https://codereview.chromium.org/2232453002/diff/140001/chrome/browser/ui/view...
chrome/browser/ui/views/website_settings/website_settings_popup_view.cc:338:
text = IDS_PAGE_INFO_VIEW_SOURCE_PAGE;
can you copy the same logic here too, with the complete if-else statement with a
NOTREACHED()?

[meacer, 2016-08-13 01:33:54]

The CQ bit was checked by meacer@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-13 01:34:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-13 02:19:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-13 02:19:10]

Dry run: This issue passed the CQ dry run.

[meacer, 2016-08-15 17:52:53]

@felt: Do latest changes LGTY?
@caitkp: sdefresne is OOO, could you please take a look at the string change in
components?

[meacer, 2016-08-15 17:53:08]

meacer@chromium.org changed reviewers:
+ caitkp@chromium.org

[felt, 2016-08-15 18:01:57]

lgtm

[meacer, 2016-08-15 21:19:57]

Description was changed from

==========
Show a custom message in the page info bubble for view-source URLs

Screenshot:
https://drive.google.com/file/d/0B9q2eN9gDoUIMGYyeE85Ynk2VGs/view?usp=sharing


BUG=635776
==========

to

==========
Show a custom message in the page info bubble for view-source URLs

Screenshot:
https://drive.google.com/file/d/0B9q2eN9gDoUIMGYyeE85Ynk2VGs/view?usp=sharing


BUG=635776
TBR=sdefresne,caitkp
==========

[meacer, 2016-08-15 21:21:12]

caitkp, sdefresne: TBRing you for the string change. Would like to land this
today as it's a stable release blocker. Thanks!

[meacer, 2016-08-15 21:21:17]

The CQ bit was checked by meacer@chromium.org

[meacer, 2016-08-15 21:21:18]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
felt@chromium.org
Link to the patchset: https://codereview.chromium.org/2232453002/#ps180001
(title: "The")

[commit-bot: I haz the power, 2016-08-15 21:21:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Cait (Slow), 2016-08-15 21:25:09]

component_strings lgtm

[commit-bot: I haz the power, 2016-08-15 22:05:01]

Description was changed from

==========
Show a custom message in the page info bubble for view-source URLs

Screenshot:
https://drive.google.com/file/d/0B9q2eN9gDoUIMGYyeE85Ynk2VGs/view?usp=sharing


BUG=635776
TBR=sdefresne,caitkp
==========

to

==========
Show a custom message in the page info bubble for view-source URLs

Screenshot:
https://drive.google.com/file/d/0B9q2eN9gDoUIMGYyeE85Ynk2VGs/view?usp=sharing


BUG=635776
TBR=sdefresne,caitkp
==========

[commit-bot: I haz the power, 2016-08-15 22:05:04]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-08-15 22:09:16]

Description was changed from

==========
Show a custom message in the page info bubble for view-source URLs

Screenshot:
https://drive.google.com/file/d/0B9q2eN9gDoUIMGYyeE85Ynk2VGs/view?usp=sharing


BUG=635776
TBR=sdefresne,caitkp
==========

to

==========
Show a custom message in the page info bubble for view-source URLs

Screenshot:
https://drive.google.com/file/d/0B9q2eN9gDoUIMGYyeE85Ynk2VGs/view?usp=sharing

BUG=635776
TBR=sdefresne,caitkp

Committed: https://crrev.com/e1feca06c43dba6d9465e9a06370cbab24f5d7fe
Cr-Commit-Position: refs/heads/master@{#412067}
==========

[commit-bot: I haz the power, 2016-08-15 22:09:18]

Patchset 10 (id:??) landed as
https://crrev.com/e1feca06c43dba6d9465e9a06370cbab24f5d7fe
Cr-Commit-Position: refs/heads/master@{#412067}