[Extensions] Show a "refresh" bubble when needed with click-to-script

[Extensions] Show a "refresh" bubble when needed with click-to-script

When an extension wants to inject scripts at document start or use webRequest,
it needs to act on the page before the page completes loading. This is a problem
with click-to-script, which makes extensions wait for user action. For now,
add a bubble that prompts the user to refresh the page in order to allow the
extension to run on the page if the user triggers it and it had blocked actions
that need to run at document_start.

BUG=460306
Committed: https://crrev.com/4a78c48bcf51dda7cb915ba1866fdefcffa9bc0c
Cr-Commit-Position: refs/heads/master@{#382981}

[Devlin, 2016-03-18 23:19:19]

Patchset #1 (id:1) has been deleted

[Devlin, 2016-03-18 23:19:26]

Patchset #1 (id:20001) has been deleted

[Devlin, 2016-03-18 23:19:32]

Patchset #1 (id:40001) has been deleted

[Devlin, 2016-03-18 23:19:38]

Patchset #1 (id:60001) has been deleted

[Devlin, 2016-03-18 23:19:44]

Patchset #1 (id:80001) has been deleted

[Devlin, 2016-03-18 23:19:49]

Patchset #1 (id:100001) has been deleted

[Devlin, 2016-03-18 23:22:12]

Description was changed from

==========
[Extensions] c2s
BUG=
==========

to

==========
[Extensions] Show a "refresh" bubble with click-to-script and document_start/wr

When an extension wants to inject scripts at document start or use webRequest,
it needs to act on the page before page completes loading. This is a problem
with click-to-script, which makes extensions wait for user action. For now,
add a bubble that prompts the user to refresh the page in order to allow the
extension to run on the page if the user triggers it and it had blocked actions
that need to run at document_start.

BUG=460306
==========

[Devlin, 2016-03-18 23:22:25]

rdevlin.cronin@chromium.org changed reviewers:
+ asargent@chromium.org

[Devlin, 2016-03-18 23:22:26]

Antony, mind taking a look?

[asargent_no_longer_on_chrome, 2016-03-21 23:31:15]

lgtm


CL description nits:

-In title, does "document_start/wr" contain a typo at the end there, or does
"/wr" stand for something? 

-"act on the page before page completes loading" - missing "the"

https://codereview.chromium.org/1809813002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/extension_action_runner.h (right):

https://codereview.chromium.org/1809813002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/extension_action_runner.h:152: // actions for the
given |extension|.
I could not help thinking of "the old lady who swallowed a fly" song when
reading this comment. =)

https://codereview.chromium.org/1809813002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/extension_action_runner_browsertest.cc (right):

https://codereview.chromium.org/1809813002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/extension_action_runner_browsertest.cc:453: const GURL
kUrl = embedded_test_server()->GetURL("/simple.html");
optional: it seems a little unusual to use the "k" prefix for a local variable;
usually I expect to look up at the top of a file or in some include file for
variables named like that. Consider modifying.

[Devlin, 2016-03-22 00:12:56]

rdevlin.cronin@chromium.org changed reviewers:
+ finnur@chromium.org

[Devlin, 2016-03-22 00:12:57]

+finnur for extensions ui

[Finnur, 2016-03-22 10:11:03]

Unfortunately, I'm unable to comment on this review, due to:
http://crbug.com/596426

:(

On Tue, Mar 22, 2016 at 12:12 AM, <rdevlin.cronin@chromium.org> wrote:

> +finnur for extensions ui
>
> https://codereview.chromium.org/1809813002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Finnur, 2016-03-22 10:24:20]

Well, I can comment through email, but boy is that painful... Fortunately,
I don't have many comments.


extensions ui LGTM, with nits and request to see a screenshot of the bubble
once the below has been addressed/dismissed:


Strings:

> In order to run this extension, we need to refresh the page.

The 'we' here is a bit creepy -- how about 'you' perhaps?

> Ok, refresh

Isn't it 'OK'?

> Nevermind

Never mind?

> GetActionId() in toolbar_actions_bar_bubble_delegate.h

Shouldn't this be named GetAnchorActionId()? It's a little generic as it
stands. And int he same file:

> bubble should point to the full container.

nit: 'whole' would be better than 'full', but either word feels redundant
to me.

On Tue, Mar 22, 2016 at 10:11 AM, Finnur Thorarinsson <finnur@chromium.org>
wrote:

> Unfortunately, I'm unable to comment on this review, due to:
> http://crbug.com/596426
>
> :(
>
> On Tue, Mar 22, 2016 at 12:12 AM, <rdevlin.cronin@chromium.org> wrote:
>
>> +finnur for extensions ui
>>
>> https://codereview.chromium.org/1809813002/
>>
>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Finnur, 2016-03-22 10:25:01]

LGTM

On Tue, Mar 22, 2016 at 10:24 AM, Finnur Thorarinsson <finnur@chromium.org>
wrote:

> Well, I can comment through email, but boy is that painful... Fortunately,
> I don't have many comments.
>
>
> extensions ui LGTM, with nits and request to see a screenshot of the
> bubble once the below has been addressed/dismissed:
>
>
> Strings:
>
> > In order to run this extension, we need to refresh the page.
>
> The 'we' here is a bit creepy -- how about 'you' perhaps?
>
> > Ok, refresh
>
> Isn't it 'OK'?
>
> > Nevermind
>
> Never mind?
>
> > GetActionId() in toolbar_actions_bar_bubble_delegate.h
>
> Shouldn't this be named GetAnchorActionId()? It's a little generic as it
> stands. And int he same file:
>
> > bubble should point to the full container.
>
> nit: 'whole' would be better than 'full', but either word feels redundant
> to me.
>
> On Tue, Mar 22, 2016 at 10:11 AM, Finnur Thorarinsson <finnur@chromium.org
> > wrote:
>
>> Unfortunately, I'm unable to comment on this review, due to:
>> http://crbug.com/596426
>>
>> :(
>>
>> On Tue, Mar 22, 2016 at 12:12 AM, <rdevlin.cronin@chromium.org> wrote:
>>
>>> +finnur for extensions ui
>>>
>>> https://codereview.chromium.org/1809813002/
>>>
>>
>>
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Finnur, 2016-03-22 10:25:15]

LGTM

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Finnur, 2016-03-22 10:38:20]

Hopefully this works. 

LGTM.

[Devlin, 2016-03-22 20:54:10]

Description was changed from

==========
[Extensions] Show a "refresh" bubble with click-to-script and document_start/wr

When an extension wants to inject scripts at document start or use webRequest,
it needs to act on the page before page completes loading. This is a problem
with click-to-script, which makes extensions wait for user action. For now,
add a bubble that prompts the user to refresh the page in order to allow the
extension to run on the page if the user triggers it and it had blocked actions
that need to run at document_start.

BUG=460306
==========

to

==========
[Extensions] Show a "refresh" bubble when needed with click-to-script

When an extension wants to inject scripts at document start or use webRequest,
it needs to act on the page before page completes loading. This is a problem
with click-to-script, which makes extensions wait for user action. For now,
add a bubble that prompts the user to refresh the page in order to allow the
extension to run on the page if the user triggers it and it had blocked actions
that need to run at document_start.

BUG=460306
==========

[Devlin, 2016-03-22 20:54:47]

Description was changed from

==========
[Extensions] Show a "refresh" bubble when needed with click-to-script

When an extension wants to inject scripts at document start or use webRequest,
it needs to act on the page before page completes loading. This is a problem
with click-to-script, which makes extensions wait for user action. For now,
add a bubble that prompts the user to refresh the page in order to allow the
extension to run on the page if the user triggers it and it had blocked actions
that need to run at document_start.

BUG=460306
==========

to

==========
[Extensions] Show a "refresh" bubble when needed with click-to-script

When an extension wants to inject scripts at document start or use webRequest,
it needs to act on the page before the page completes loading. This is a problem
with click-to-script, which makes extensions wait for user action. For now,
add a bubble that prompts the user to refresh the page in order to allow the
extension to run on the page if the user triggers it and it had blocked actions
that need to run at document_start.

BUG=460306
==========

[Devlin, 2016-03-23 21:46:52]

Patchset #2 (id:140001) has been deleted

[Devlin, 2016-03-23 21:46:59]

Patchset #2 (id:160001) has been deleted

[Devlin, 2016-03-23 21:47:08]

Patchset #2 (id:180001) has been deleted

[Devlin, 2016-03-23 21:48:46]

On 2016/03/22 10:24:20, Finnur wrote:
> Well, I can comment through email, but boy is that painful... Fortunately,
> I don't have many comments.

Thanks for doing this!  Sorry it was a painful process - hopefully it's fixed
soon.

> extensions ui LGTM, with nits and request to see a screenshot of the bubble
> once the below has been addressed/dismissed:
I've attached a screenshot of the bubble to the bug (still waiting for rietveld
to have file upload ;))

> Strings:
> > In order to run this extension, we need to refresh the page.
> The 'we' here is a bit creepy -- how about 'you' perhaps?
Done.

> > Ok, refresh
> Isn't it 'OK'?
$ git grep "Ok" chrome/app/generated_resources.grd | wc -l
49
$ git grep "OK" chrome/app/generated_resources.grd | wc -l
218

Looks like 'OK' is more popular, but apparently we are inconsistent (happy to
use 'OK' here though since that's more common one).

> > Nevermind
> Never mind?
News to me!  And no strings in chrome to compare to. But a google search seems
to side with you. :)  Done.

> > GetActionId() in toolbar_actions_bar_bubble_delegate.h
> Shouldn't this be named GetAnchorActionId()? It's a little generic as it
> stands. And int he same file:
Good call; much clearer that way.  Done.

> > bubble should point to the full container.
> nit: 'whole' would be better than 'full', but either word feels redundant
> to me.
Hmm... how about "point to the center of the"?  I think that's the clearest of
all.

On 2016/03/21 23:31:15, Antony Sargent wrote:
> CL description nits:
>
> -In title, does "document_start/wr" contain a typo at the end there, or does
> "/wr" stand for something?
Stands for webRequest - couldn't fit with the 80 char limit. Rephrased.

> -"act on the page before page completes loading" - missing "the"
Done

https://codereview.chromium.org/1809813002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/extension_action_runner.h (right):

https://codereview.chromium.org/1809813002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/extension_action_runner.h:152: // actions for the
given |extension|.
On 2016/03/21 23:31:15, Antony Sargent wrote:
> I could not help thinking of "the old lady who swallowed a fly" song when
> reading this comment. =)
> 

heh hopefully it doesn't get any more complicated than that :)

https://codereview.chromium.org/1809813002/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/extension_action_runner_browsertest.cc (right):

https://codereview.chromium.org/1809813002/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/extension_action_runner_browsertest.cc:453: const GURL
kUrl = embedded_test_server()->GetURL("/simple.html");
On 2016/03/21 23:31:15, Antony Sargent wrote:
> optional: it seems a little unusual to use the "k" prefix for a local
variable;
> usually I expect to look up at the top of a file or in some include file for
> variables named like that. Consider modifying. 

I know we do it elsewhere, but I'm not sure if there's a sane rule.  Happy to
change it, though, so done.

[Devlin, 2016-03-23 21:49:08]

rdevlin.cronin@chromium.org changed reviewers:
+ avi@chromium.org

[Devlin, 2016-03-23 21:49:09]

+Avi for the cocoa

[Avi (use Gerrit), 2016-03-23 22:05:24]

The code is fine but the comment makes no sense.

https://codereview.chromium.org/1809813002/diff/200001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right):

https://codereview.chromium.org/1809813002/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:171: // menu if
no |anchorAction| is null.
This comment made no sense with the previous version of the function
declaration, and it makes just as no sense with this new version.

Can you reword it to be applicable?

[Devlin, 2016-03-23 22:13:36]

Patchset #3 (id:220001) has been deleted

[Devlin, 2016-03-23 22:23:09]

https://codereview.chromium.org/1809813002/diff/200001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right):

https://codereview.chromium.org/1809813002/diff/200001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:171: // menu if
no |anchorAction| is null.
On 2016/03/23 22:05:24, Avi wrote:
> This comment made no sense with the previous version of the function
> declaration, and it makes just as no sense with this new version.
> 
> Can you reword it to be applicable?

Whoops!  Done.

[Avi (use Gerrit), 2016-03-23 22:34:15]

LGTM

[Devlin, 2016-03-23 22:43:44]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2016-03-23 22:43:44]

The patchset sent to the CQ was uploaded after l-g-t-m from finnur@chromium.org,
asargent@chromium.org
Link to the patchset: https://codereview.chromium.org/1809813002/#ps230001
(title: "Update comment")

[commit-bot: I haz the power, 2016-03-23 22:44:09]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1809813002/230001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1809813002/230001

[commit-bot: I haz the power, 2016-03-24 00:02:39]

Description was changed from

==========
[Extensions] Show a "refresh" bubble when needed with click-to-script

When an extension wants to inject scripts at document start or use webRequest,
it needs to act on the page before the page completes loading. This is a problem
with click-to-script, which makes extensions wait for user action. For now,
add a bubble that prompts the user to refresh the page in order to allow the
extension to run on the page if the user triggers it and it had blocked actions
that need to run at document_start.

BUG=460306
==========

to

==========
[Extensions] Show a "refresh" bubble when needed with click-to-script

When an extension wants to inject scripts at document start or use webRequest,
it needs to act on the page before the page completes loading. This is a problem
with click-to-script, which makes extensions wait for user action. For now,
add a bubble that prompts the user to refresh the page in order to allow the
extension to run on the page if the user triggers it and it had blocked actions
that need to run at document_start.

BUG=460306
==========

[commit-bot: I haz the power, 2016-03-24 00:02:44]

Committed patchset #3 (id:230001)

[commit-bot: I haz the power, 2016-03-24 00:04:00]

Description was changed from

==========
[Extensions] Show a "refresh" bubble when needed with click-to-script

When an extension wants to inject scripts at document start or use webRequest,
it needs to act on the page before the page completes loading. This is a problem
with click-to-script, which makes extensions wait for user action. For now,
add a bubble that prompts the user to refresh the page in order to allow the
extension to run on the page if the user triggers it and it had blocked actions
that need to run at document_start.

BUG=460306
==========

to

==========
[Extensions] Show a "refresh" bubble when needed with click-to-script

When an extension wants to inject scripts at document start or use webRequest,
it needs to act on the page before the page completes loading. This is a problem
with click-to-script, which makes extensions wait for user action. For now,
add a bubble that prompts the user to refresh the page in order to allow the
extension to run on the page if the user triggers it and it had blocked actions
that need to run at document_start.

BUG=460306

Committed: https://crrev.com/4a78c48bcf51dda7cb915ba1866fdefcffa9bc0c
Cr-Commit-Position: refs/heads/master@{#382981}
==========

[commit-bot: I haz the power, 2016-03-24 00:04:01]

Patchset 3 (id:??) landed as
https://crrev.com/4a78c48bcf51dda7cb915ba1866fdefcffa9bc0c
Cr-Commit-Position: refs/heads/master@{#382981}