Remove extension group from DOMWrapperWorld.

Remove extension group from DOMWrapperWorld.

Presumably this was used in the past, but it isn't used anymore except
in the ScriptContextSet classifier. However, the classifier can use the
world ID to accomplish the same thing, so just remove this and clean up
a bunch of dead code.

BUG=481699
R=haraken@chromium.org,peria@chromium.org,yukishiino@chromium.org
TBR=jochen@chromium.org,rdevlin.cronin@chromium.org

Review-Url: https://codereview.chromium.org/2628053003
Cr-Commit-Position: refs/heads/master@{#443498}
Committed: https://chromium.googlesource.com/chromium/src/+/05529947077574fb6d7ebd4e0a9ced6159847fc9

[dcheng, 2017-01-11 22:34:19]

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

[commit-bot: I haz the power, 2017-01-11 22:36:00]

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, 2017-01-12 01:00:58]

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

[commit-bot: I haz the power, 2017-01-12 01:00:59]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-01-12 01:39:52]

dcheng@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org, yukishiino@chromium.org

[dcheng, 2017-01-12 01:39:53]

This removes all the plumbing for extension group. No tests seem to break... so
yay?

This paves the way for cleaning up the several things related to DOMWrapperWorld
in Blink.

https://codereview.chromium.org/2628053003/diff/1/extensions/renderer/script_...
File extensions/renderer/script_context_set.cc (right):

https://codereview.chromium.org/2628053003/diff/1/extensions/renderer/script_...
extensions/renderer/script_context_set.cc:178: // for content script.
This is the only real functional change in this CL. I'm not 100% sure this
statement is always true, but all the tests seem OK with it.

[haraken, 2017-01-12 01:53:45]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2017-01-12 01:53:45]

LGTM on my side

[peria, 2017-01-12 02:04:51]

LGTM. Yay!

[Yuki, 2017-01-12 03:31:12]

LGTM.  Yay!!

[Devlin, 2017-01-12 16:30:12]

Nice cleanup!  One small, but important, correction.

https://codereview.chromium.org/2628053003/diff/1/extensions/renderer/script_...
File extensions/renderer/script_context_set.cc (right):

https://codereview.chromium.org/2628053003/diff/1/extensions/renderer/script_...
extensions/renderer/script_context_set.cc:178: // for content script.
On 2017/01/12 01:39:52, dcheng wrote:
> This is the only real functional change in this CL. I'm not 100% sure this
> statement is always true, but all the tests seem OK with it.

This isn't quite correct - instead, it should be:
if (world_id >= ExtensionsRendererClient::Get()->GetLowestIsolatedWorldId())

Otherwise, we'll categorize translate and chrome-internal (dom distiller)
isolated worlds as content script contexts.

It would be an interesting test to make sure that doesn't happen, but I'm not
sure what the best way to do that would be...

[dcheng, 2017-01-12 18:35:13]

https://codereview.chromium.org/2628053003/diff/1/extensions/renderer/script_...
File extensions/renderer/script_context_set.cc (right):

https://codereview.chromium.org/2628053003/diff/1/extensions/renderer/script_...
extensions/renderer/script_context_set.cc:178: // for content script.
On 2017/01/12 16:30:12, Devlin wrote:
> On 2017/01/12 01:39:52, dcheng wrote:
> > This is the only real functional change in this CL. I'm not 100% sure this
> > statement is always true, but all the tests seem OK with it.
> 
> This isn't quite correct - instead, it should be:
> if (world_id >= ExtensionsRendererClient::Get()->GetLowestIsolatedWorldId())
> 
> Otherwise, we'll categorize translate and chrome-internal (dom distiller)
> isolated worlds as content script contexts.
> 
> It would be an interesting test to make sure that doesn't happen, but I'm not
> sure what the best way to do that would be...

Looking at this, I'm not actually sure what the right behavior is.

Before we call ClassifyJavascriptContext(), we call
GetExtensionFromFrameAndWorld().

The implementation of GetExtensionFromFrameAndWorld() also checks that world id
!= 0. If != 0, it assumes content script and tries to look up an extension. I
suppose that if it's for a chrome internal world, the extension lookup would
fail because it simply won't be in the map.

Then we end up here, and the logic nicely lines up -- if world_id != 0 but we
have no extension, we'll end up with an UNSPECIFIED_CONTEXT... I think. So I
guess I can remove kalman's TODO here and update the comment... but I think this
logic might actually be (unintentionally) right.

[Devlin, 2017-01-12 21:31:39]

https://codereview.chromium.org/2628053003/diff/1/extensions/renderer/script_...
File extensions/renderer/script_context_set.cc (right):

https://codereview.chromium.org/2628053003/diff/1/extensions/renderer/script_...
extensions/renderer/script_context_set.cc:178: // for content script.
On 2017/01/12 18:35:12, dcheng wrote:
> On 2017/01/12 16:30:12, Devlin wrote:
> > On 2017/01/12 01:39:52, dcheng wrote:
> > > This is the only real functional change in this CL. I'm not 100% sure this
> > > statement is always true, but all the tests seem OK with it.
> > 
> > This isn't quite correct - instead, it should be:
> > if (world_id >= ExtensionsRendererClient::Get()->GetLowestIsolatedWorldId())
> > 
> > Otherwise, we'll categorize translate and chrome-internal (dom distiller)
> > isolated worlds as content script contexts.
> > 
> > It would be an interesting test to make sure that doesn't happen, but I'm
not
> > sure what the best way to do that would be...
> 
> Looking at this, I'm not actually sure what the right behavior is.
> 
> Before we call ClassifyJavascriptContext(), we call
> GetExtensionFromFrameAndWorld().
> 
> The implementation of GetExtensionFromFrameAndWorld() also checks that world
id
> != 0. If != 0, it assumes content script and tries to look up an extension. I
> suppose that if it's for a chrome internal world, the extension lookup would
> fail because it simply won't be in the map.
> 
> Then we end up here, and the logic nicely lines up -- if world_id != 0 but we
> have no extension, we'll end up with an UNSPECIFIED_CONTEXT... I think. So I
> guess I can remove kalman's TODO here and update the comment... but I think
this
> logic might actually be (unintentionally) right.

So, a couple of things here.

Unspecified contexts should be avoided.  They're cop-outs. :)  For our sake, we
only care about extension-related contexts and webui, so if it's not one of
those, it should be considered WEB_PAGE_CONTEXT.  Previously, this logic said
UNSPECIFIED because we had one piece of information (extension group) saying
"this is extensiony" and another saying "no it's not" - so we threw our hands in
the air.  Here, having a non-zero world id doesn't indicate extension-ness, so
we shouldn't assume it.

GetExtensionFromFrameAndWorld() does the wrong thing in checking 0.  It should
also check minimum isolated world for extensions.  I'll write a patch to change
that.  I even wonder if that could explain some of the "when does this happens?"
in the code.

[dcheng, 2017-01-12 21:40:41]

https://codereview.chromium.org/2628053003/diff/1/extensions/renderer/script_...
File extensions/renderer/script_context_set.cc (right):

https://codereview.chromium.org/2628053003/diff/1/extensions/renderer/script_...
extensions/renderer/script_context_set.cc:178: // for content script.
On 2017/01/12 21:31:39, Devlin wrote:
> On 2017/01/12 18:35:12, dcheng wrote:
> > On 2017/01/12 16:30:12, Devlin wrote:
> > > On 2017/01/12 01:39:52, dcheng wrote:
> > > > This is the only real functional change in this CL. I'm not 100% sure
this
> > > > statement is always true, but all the tests seem OK with it.
> > > 
> > > This isn't quite correct - instead, it should be:
> > > if (world_id >=
ExtensionsRendererClient::Get()->GetLowestIsolatedWorldId())
> > > 
> > > Otherwise, we'll categorize translate and chrome-internal (dom distiller)
> > > isolated worlds as content script contexts.
> > > 
> > > It would be an interesting test to make sure that doesn't happen, but I'm
> not
> > > sure what the best way to do that would be...
> > 
> > Looking at this, I'm not actually sure what the right behavior is.
> > 
> > Before we call ClassifyJavascriptContext(), we call
> > GetExtensionFromFrameAndWorld().
> > 
> > The implementation of GetExtensionFromFrameAndWorld() also checks that world
> id
> > != 0. If != 0, it assumes content script and tries to look up an extension.
I
> > suppose that if it's for a chrome internal world, the extension lookup would
> > fail because it simply won't be in the map.
> > 
> > Then we end up here, and the logic nicely lines up -- if world_id != 0 but
we
> > have no extension, we'll end up with an UNSPECIFIED_CONTEXT... I think. So I
> > guess I can remove kalman's TODO here and update the comment... but I think
> this
> > logic might actually be (unintentionally) right.
> 
> So, a couple of things here.
> 
> Unspecified contexts should be avoided.  They're cop-outs. :)  For our sake,
we
> only care about extension-related contexts and webui, so if it's not one of
> those, it should be considered WEB_PAGE_CONTEXT.  Previously, this logic said
> UNSPECIFIED because we had one piece of information (extension group) saying
> "this is extensiony" and another saying "no it's not" - so we threw our hands
in
> the air.  Here, having a non-zero world id doesn't indicate extension-ness, so
> we shouldn't assume it.
> 
> GetExtensionFromFrameAndWorld() does the wrong thing in checking 0.  It should
> also check minimum isolated world for extensions.  I'll write a patch to
change
> that.  I even wonder if that could explain some of the "when does this
happens?"
> in the code.

It could, but I'm not sure the burden of fixing that should be in this patch.

Without fixing that, I'm still going to need to use UNSPECIFIED_CONTEXT here.
Currently, we (incorrectly it sounds like) don't early return if it's not
actually an extension context, so we *must* do something here.

[Devlin, 2017-01-12 22:02:09]

https://codereview.chromium.org/2628053003/diff/1/extensions/renderer/script_...
File extensions/renderer/script_context_set.cc (right):

https://codereview.chromium.org/2628053003/diff/1/extensions/renderer/script_...
extensions/renderer/script_context_set.cc:178: // for content script.
On 2017/01/12 21:40:41, dcheng wrote:
> On 2017/01/12 21:31:39, Devlin wrote:
> > On 2017/01/12 18:35:12, dcheng wrote:
> > > On 2017/01/12 16:30:12, Devlin wrote:
> > > > On 2017/01/12 01:39:52, dcheng wrote:
> > > > > This is the only real functional change in this CL. I'm not 100% sure
> this
> > > > > statement is always true, but all the tests seem OK with it.
> > > > 
> > > > This isn't quite correct - instead, it should be:
> > > > if (world_id >=
> ExtensionsRendererClient::Get()->GetLowestIsolatedWorldId())
> > > > 
> > > > Otherwise, we'll categorize translate and chrome-internal (dom
distiller)
> > > > isolated worlds as content script contexts.
> > > > 
> > > > It would be an interesting test to make sure that doesn't happen, but
I'm
> > not
> > > > sure what the best way to do that would be...
> > > 
> > > Looking at this, I'm not actually sure what the right behavior is.
> > > 
> > > Before we call ClassifyJavascriptContext(), we call
> > > GetExtensionFromFrameAndWorld().
> > > 
> > > The implementation of GetExtensionFromFrameAndWorld() also checks that
world
> > id
> > > != 0. If != 0, it assumes content script and tries to look up an
extension.
> I
> > > suppose that if it's for a chrome internal world, the extension lookup
would
> > > fail because it simply won't be in the map.
> > > 
> > > Then we end up here, and the logic nicely lines up -- if world_id != 0 but
> we
> > > have no extension, we'll end up with an UNSPECIFIED_CONTEXT... I think. So
I
> > > guess I can remove kalman's TODO here and update the comment... but I
think
> > this
> > > logic might actually be (unintentionally) right.
> > 
> > So, a couple of things here.
> > 
> > Unspecified contexts should be avoided.  They're cop-outs. :)  For our sake,
> we
> > only care about extension-related contexts and webui, so if it's not one of
> > those, it should be considered WEB_PAGE_CONTEXT.  Previously, this logic
said
> > UNSPECIFIED because we had one piece of information (extension group) saying
> > "this is extensiony" and another saying "no it's not" - so we threw our
hands
> in
> > the air.  Here, having a non-zero world id doesn't indicate extension-ness,
so
> > we shouldn't assume it.
> > 
> > GetExtensionFromFrameAndWorld() does the wrong thing in checking 0.  It
should
> > also check minimum isolated world for extensions.  I'll write a patch to
> change
> > that.  I even wonder if that could explain some of the "when does this
> happens?"
> > in the code.
> 
> It could, but I'm not sure the burden of fixing that should be in this patch.
> 
> Without fixing that, I'm still going to need to use UNSPECIFIED_CONTEXT here.
> Currently, we (incorrectly it sounds like) don't early return if it's not
> actually an extension context, so we *must* do something here.

There is a behavior change from the current behavior in this patch.

currently:
bool do_we_think_this_is_an_extension = extension_group ==
EXTENSION_GROUP_CONTENT_SCRIPTS;

with this patch:
bool do_we_think_this_is_an_extension = world_id != 0;

However, if we have a context for e.g. translate or chrome internal, this new
version will classify that as UNSPECIFIED_CONTEXT - because world_id != 0, and
there's no |extension|.  Previously (before this patch), it would classify this
as WEB_PAGE_CONTEXT, because the extension_group would not be
EXTENSION_GROUP_CONTENT_SCRIPTS.

Does that make sense?  Feel free to ping with questions. :)

[dcheng, 2017-01-12 22:56:06]

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

[commit-bot: I haz the power, 2017-01-12 22:56:41]

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

[dcheng, 2017-01-12 22:58:01]

PTAL. I tried to write a test for this, but ScriptContextSet depends on too many
statics and lazy instances to easily make this all work together: this otherwise
simple unit test would have to bootstrap a full RenderFrame, register test
extensions, and fake out script injection.

So... it's untested.

[Devlin, 2017-01-12 23:00:10]

extensions lgtm.  Thanks :)

[dcheng, 2017-01-12 23:01:57]

The CQ bit was unchecked by dcheng@chromium.org

[dcheng, 2017-01-12 23:01:57]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2017-01-12 23:01:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, peria@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2628053003/#ps20001
(title: ".")

[commit-bot: I haz the power, 2017-01-12 23:03:20]

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

[dcheng, 2017-01-12 23:10:30]

Description was changed from

==========
Remove extension group from DOMWrapperWorld.

Presumably this was used in the past, but it isn't used anymore except
in the ScriptContextSet classifier. However, the classifier can use the
world ID to accomplish the same thing, so just remove this and clean up
a bunch of dead code.

BUG=481699
==========

to

==========
Remove extension group from DOMWrapperWorld.

Presumably this was used in the past, but it isn't used anymore except
in the ScriptContextSet classifier. However, the classifier can use the
world ID to accomplish the same thing, so just remove this and clean up
a bunch of dead code.

BUG=481699
R=haraken@chromium.org,peria@chromium.org,yukishiino@chromium.org
TBR=jochen@chromium.org
==========

[dcheng, 2017-01-12 23:10:30]

dcheng@chromium.org changed reviewers:
+ jochen@chromium.org

[dcheng, 2017-01-12 23:10:40]

TBRing jochen for:
    chrome/renderer/chrome_render_frame_observer.cc
    chrome/renderer/translate/translate_helper_browsertest.cc
   
components/dom_distiller/content/renderer/distiller_js_render_frame_observer.cc
   
components/dom_distiller/content/renderer/distiller_js_render_frame_observer.h
    components/test_runner/test_runner_for_specific_view.cc
    components/translate/content/renderer/translate_helper.cc
    components/translate/content/renderer/translate_helper.h
    content/public/renderer/render_frame_observer.h
    content/renderer/dom_automation_controller.cc
    content/renderer/dom_automation_controller.h
    content/renderer/render_frame_impl.cc
    content/renderer/render_frame_impl.h
    content/renderer/render_view_browsertest.cc

[dcheng, 2017-01-12 23:10:48]

The CQ bit was unchecked by dcheng@chromium.org

[dcheng, 2017-01-12 23:10:48]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2017-01-12 23:11:25]

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, 2017-01-13 00:27:00]

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

[commit-bot: I haz the power, 2017-01-13 00:27:01]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dcheng, 2017-01-13 00:44:55]

The CQ bit was checked by dcheng@chromium.org

[commit-bot: I haz the power, 2017-01-13 00:45:42]

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, 2017-01-13 00:56:20]

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

[commit-bot: I haz the power, 2017-01-13 00:56:23]

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_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dcheng, 2017-01-13 03:29:33]

Description was changed from

==========
Remove extension group from DOMWrapperWorld.

Presumably this was used in the past, but it isn't used anymore except
in the ScriptContextSet classifier. However, the classifier can use the
world ID to accomplish the same thing, so just remove this and clean up
a bunch of dead code.

BUG=481699
R=haraken@chromium.org,peria@chromium.org,yukishiino@chromium.org
TBR=jochen@chromium.org
==========

to

==========
Remove extension group from DOMWrapperWorld.

Presumably this was used in the past, but it isn't used anymore except
in the ScriptContextSet classifier. However, the classifier can use the
world ID to accomplish the same thing, so just remove this and clean up
a bunch of dead code.

BUG=481699
R=haraken@chromium.org,peria@chromium.org,rdevlin.cronin@chromium.org,yukishi...
TBR=jochen@chromium.org
==========

[dcheng, 2017-01-13 03:30:02]

TBRing rdevlin for a small fix to another extensions test that uses
ScriptContextSet (just adds a TestExtensionsRendererClient to the test fixture).

[dcheng, 2017-01-13 03:30:19]

Description was changed from

==========
Remove extension group from DOMWrapperWorld.

Presumably this was used in the past, but it isn't used anymore except
in the ScriptContextSet classifier. However, the classifier can use the
world ID to accomplish the same thing, so just remove this and clean up
a bunch of dead code.

BUG=481699
R=haraken@chromium.org,peria@chromium.org,rdevlin.cronin@chromium.org,yukishi...
TBR=jochen@chromium.org
==========

to

==========
Remove extension group from DOMWrapperWorld.

Presumably this was used in the past, but it isn't used anymore except
in the ScriptContextSet classifier. However, the classifier can use the
world ID to accomplish the same thing, so just remove this and clean up
a bunch of dead code.

BUG=481699
R=haraken@chromium.org,peria@chromium.org,yukishiino@chromium.org
TBR=jochen@chromium.org,rdevlin.cronin@chromium.org
==========

[dcheng, 2017-01-13 03:30:22]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2017-01-13 03:30:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, rdevlin.cronin@chromium.org, peria@chromium.org,
yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2628053003/#ps40001
(title: "Fix GCCallbackTest")

[commit-bot: I haz the power, 2017-01-13 03:31:25]

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, 2017-01-13 05:35:43]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1484278222944500,
"parent_rev": "bea6ff95471b7a3da8030b0903d73e48434cc18a", "commit_rev":
"05529947077574fb6d7ebd4e0a9ced6159847fc9"}

[commit-bot: I haz the power, 2017-01-13 05:36:17]

Description was changed from

==========
Remove extension group from DOMWrapperWorld.

Presumably this was used in the past, but it isn't used anymore except
in the ScriptContextSet classifier. However, the classifier can use the
world ID to accomplish the same thing, so just remove this and clean up
a bunch of dead code.

BUG=481699
R=haraken@chromium.org,peria@chromium.org,yukishiino@chromium.org
TBR=jochen@chromium.org,rdevlin.cronin@chromium.org
==========

to

==========
Remove extension group from DOMWrapperWorld.

Presumably this was used in the past, but it isn't used anymore except
in the ScriptContextSet classifier. However, the classifier can use the
world ID to accomplish the same thing, so just remove this and clean up
a bunch of dead code.

BUG=481699
R=haraken@chromium.org,peria@chromium.org,yukishiino@chromium.org
TBR=jochen@chromium.org,rdevlin.cronin@chromium.org

Review-Url: https://codereview.chromium.org/2628053003
Cr-Commit-Position: refs/heads/master@{#443498}
Committed:
https://chromium.googlesource.com/chromium/src/+/05529947077574fb6d7ebd4e0a9c...
==========

[commit-bot: I haz the power, 2017-01-13 05:36:18]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/05529947077574fb6d7ebd4e0a9c...