webRequest: reliably determine tabId

webRequest: reliably determine tabId

BUG=613718
TEST=Provided by https://codereview.chromium.org/2003593002/.
 Tested locally with N=50 that the tests don't fail any more
 (see bug report for details).

Committed: https://crrev.com/1b8747c4fed55e7fde4344eb0a71463855096502
Cr-Commit-Position: refs/heads/master@{#395471}

[robwu, 2016-05-21 15:21:33]

rob@robwu.nl changed reviewers:
+ rdevlin.cronin@chromium.org

[Devlin, 2016-05-23 16:23:18]

https://codereview.chromium.org/2002763003/diff/1/extensions/browser/extensio...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/2002763003/diff/1/extensions/browser/extensio...
extensions/browser/extension_api_frame_id_map.cc:138:
ExtensionApiFrameIdMapHelper* helper =
(if we keep this, dcheck ui thread)

Why do we do this instead of (possibly caching and) returning the FrameData?  If
there isn't a cached frame data, there should be, since we'll probably need it
later.

[robwu, 2016-05-23 17:55:55]

https://codereview.chromium.org/2002763003/diff/1/extensions/browser/extensio...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/2002763003/diff/1/extensions/browser/extensio...
extensions/browser/extension_api_frame_id_map.cc:138:
ExtensionApiFrameIdMapHelper* helper =
On 2016/05/23 16:23:18, Devlin wrote:
> (if we keep this, dcheck ui thread)
> 
> Why do we do this instead of (possibly caching and) returning the FrameData? 
If
> there isn't a cached frame data, there should be, since we'll probably need it
> later.

I didn't realize that the tab ID was immediately set as a part of the lookup
since the documentation in the header file only mentioned tabId.
I've fixed the inaccurate comments and used FrameData instead.

[Devlin, 2016-05-23 20:28:48]

lgtm % nits

https://codereview.chromium.org/2002763003/diff/20001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_event_details.cc (right):

https://codereview.chromium.org/2002763003/diff/20001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_event_details.cc:152: void
WebRequestEventDetails::DetermineFrameIdOnUI() {
nit: this does a bit more now - maybe rename to something like
DetermineFrameData?

https://codereview.chromium.org/2002763003/diff/20001/extensions/browser/exte...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/2002763003/diff/20001/extensions/browser/exte...
extensions/browser/extension_api_frame_id_map.cc:314: return
LookupFrameDataOnUI(key, true /* for lookup */);
In retrospect, this was probably a poorly named boolean.  The idea here is that
we're recording how often we find the value for a frame after a cache miss
(indicating the value of performing the thread hop).  Since there was no initial
cache miss here, I'd prefer not to record for this case.  It would probably be
good to also rename the boolean to something like "for_lookup_after_cache_miss"
to make this more clear.

[robwu, 2016-05-23 20:54:15]

https://codereview.chromium.org/2002763003/diff/20001/extensions/browser/api/...
File extensions/browser/api/web_request/web_request_event_details.cc (right):

https://codereview.chromium.org/2002763003/diff/20001/extensions/browser/api/...
extensions/browser/api/web_request/web_request_event_details.cc:152: void
WebRequestEventDetails::DetermineFrameIdOnUI() {
On 2016/05/23 20:28:48, Devlin wrote:
> nit: this does a bit more now - maybe rename to something like
> DetermineFrameData?

Done. I also did another pass through the ExtensionApiFrameIdMap files and made
sure that everything that the methods and comments are talking about frame data
instead of frame IDs.

https://codereview.chromium.org/2002763003/diff/20001/extensions/browser/exte...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/2002763003/diff/20001/extensions/browser/exte...
extensions/browser/extension_api_frame_id_map.cc:314: return
LookupFrameDataOnUI(key, true /* for lookup */);
On 2016/05/23 20:28:48, Devlin wrote:
> In retrospect, this was probably a poorly named boolean.  The idea here is
that
> we're recording how often we find the value for a frame after a cache miss
> (indicating the value of performing the thread hop).  Since there was no
initial
> cache miss here, I'd prefer not to record for this case.  It would probably be
> good to also rename the boolean to something like
"for_lookup_after_cache_miss"
> to make this more clear.

Done. for_lookup_after_cache_miss was a bit too long for my like, so I went with
is_from_io instead.

[Devlin, 2016-05-23 20:56:12]

https://codereview.chromium.org/2002763003/diff/20001/extensions/browser/exte...
File extensions/browser/extension_api_frame_id_map.cc (right):

https://codereview.chromium.org/2002763003/diff/20001/extensions/browser/exte...
extensions/browser/extension_api_frame_id_map.cc:314: return
LookupFrameDataOnUI(key, true /* for lookup */);
On 2016/05/23 20:54:15, robwu wrote:
> On 2016/05/23 20:28:48, Devlin wrote:
> > In retrospect, this was probably a poorly named boolean.  The idea here is
> that
> > we're recording how often we find the value for a frame after a cache miss
> > (indicating the value of performing the thread hop).  Since there was no
> initial
> > cache miss here, I'd prefer not to record for this case.  It would probably
be
> > good to also rename the boolean to something like
> "for_lookup_after_cache_miss"
> > to make this more clear.
> 
> Done. for_lookup_after_cache_miss was a bit too long for my like, so I went
with
> is_from_io instead.

sgtm

[robwu, 2016-05-23 22:36:56]

The CQ bit was checked by rob@robwu.nl

[robwu, 2016-05-23 22:36:57]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/2002763003/#ps40001
(title: "Last nits")

[commit-bot: I haz the power, 2016-05-23 22:37:41]

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

[commit-bot: I haz the power, 2016-05-23 23:58:03]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-23 23:59:59]

Description was changed from

==========
webRequest: reliably determine tabId

BUG=613718
TEST=Provided by https://codereview.chromium.org/2003593002/.
 Tested locally with N=50 that the tests don't fail any more
 (see bug report for details).
==========

to

==========
webRequest: reliably determine tabId

BUG=613718
TEST=Provided by https://codereview.chromium.org/2003593002/.
 Tested locally with N=50 that the tests don't fail any more
 (see bug report for details).

Committed: https://crrev.com/1b8747c4fed55e7fde4344eb0a71463855096502
Cr-Commit-Position: refs/heads/master@{#395471}
==========

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

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