Remove chrome.* Function Instrumentation Inlining and Use a Separate Instrumented Object

Remove chrome.* Function Instrumentation Inlining and Use a Separate Instrumented Object

Chrome rebinds all of the chrome.* functions during events like permission
updates (e.g. chrome.permissions.request will cause all chrome.* to rebind).
This has the effect of throwing away all of the instrumentation set up during
initialization.

The fix here removes all inlining instrumentation from chrome.* and introduces
instrumented.* which mirrors chrome.* only for the instrumented functions.

instrumented.* also delays the lookup of the function until execution time
in the event the functions are rebound by chrome.

BUG=164227
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=216713

[vadimt, 2013-08-08 01:15:10]

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
File chrome/browser/resources/google_now/background.js (left):

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:609:
googleGeolocationAccessEnabledPref.set({value: true});
Nothing still prevents from having googleGeolocationAccessEnabledPref. Just
point it at the instrumented API.

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:254: function(items) {
You could also add newline before ['notificationsData', 'recentDismissals'],
function(items) {

This way, you wouldn't have to shift the whole function's body.

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:873:
instrumented.runtime.onInstalled.addListener(function(details) {
So, Chrome recognizes this call and loads the page on install with this event?

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:880:
instrumented.runtime.onStartup.addListener(function() {
Same here.

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
File chrome/browser/resources/google_now/utility.js (left):

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/utility.js:348: var storage =
chrome.storage.local;
Why not  var storage = instrumented.storage.local;
?

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/utility.js:312: var functioncontainer =
chrome;
chromeContainer?

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/utility.js:313: var instrumentedcontainer =
instrumented;
camelCased

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/utility.js:331: targetFunction = null;
It could be better to move the above piece of code into a separate function
where targetFunction would be a local variable. This would also solve the
problem of accidentally using it in the callback and the problem of the growing
side of instrumentChromeApiFunction.

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/utility.js:338: debugAlert('Argument ' +
callbackParameter + ' of ' + functionName +
functionIdentifier would give more information than functionName.

[robliao, 2013-08-08 17:43:31]

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
File chrome/browser/resources/google_now/background.js (left):

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:609:
googleGeolocationAccessEnabledPref.set({value: true});
True, but it wasn't universally applicable (e.g.
chrome.preferencesPrivate.googleGeolocationAccessEnabled.set).

Best not to have it to keep things explicit. It's only called three times at any
rate.
On 2013/08/08 01:15:10, vadimt wrote:
> Nothing still prevents from having googleGeolocationAccessEnabledPref. Just
> point it at the instrumented API.

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:254: function(items) {
I would rather pay for the indentation here rather than to artificially break up
the array.

On 2013/08/08 01:15:10, vadimt wrote:
> You could also add newline before ['notificationsData', 'recentDismissals'],
> function(items) {
> 
> This way, you wouldn't have to shift the whole function's body.

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:873:
instrumented.runtime.onInstalled.addListener(function(details) {
It appears so. Right after launch the event page is not in the inactive state.

I suspect it is keying off of our extension manifest to determine what to do.
The background directive in the manifest is sufficient to do the initial load.

http://developer.chrome.com/extensions/event_pages.html

The event page is loaded when it is "needed", and unloaded when it goes idle
again. Here are some examples of things that will cause the event page to load:

The app or extension is first installed or is updated to a new version (in order
to register for events).
The event page was listening for an event, and the event is dispatched.
A content script or other extension sends a message.
Another view in the extension (for example, a popup) calls
runtime.getBackgroundPage.
On 2013/08/08 01:15:10, vadimt wrote:
> So, Chrome recognizes this call and loads the page on install with this event?

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:880:
instrumented.runtime.onStartup.addListener(function() {
See above
On 2013/08/08 01:15:10, vadimt wrote:
> Same here.

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
File chrome/browser/resources/google_now/utility.js (left):

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/utility.js:348: var storage =
chrome.storage.local;
instrumented.storage.local doesn't exist until after buildTaskManager.

We'd have to instantiate buildTaskManager here for this to work.
On 2013/08/08 01:15:10, vadimt wrote:
> Why not  var storage = instrumented.storage.local;
> ?

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/utility.js:312: var functioncontainer =
chrome;
On 2013/08/08 01:15:10, vadimt wrote:
> chromeContainer?

Done.

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/utility.js:313: var instrumentedcontainer =
instrumented;
On 2013/08/08 01:15:10, vadimt wrote:
> camelCased

Done.

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/utility.js:331: targetFunction = null;
On 2013/08/08 01:15:10, vadimt wrote:
> It could be better to move the above piece of code into a separate function
> where targetFunction would be a local variable. This would also solve the
> problem of accidentally using it in the callback and the problem of the
growing
> side of instrumentChromeApiFunction.

Done.

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/utility.js:338: debugAlert('Argument ' +
callbackParameter + ' of ' + functionName +
On 2013/08/08 01:15:10, vadimt wrote:
> functionIdentifier would give more information than functionName.

Done.

[vadimt, 2013-08-08 18:19:25]

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:254: function(items) {
On 2013/08/08 17:43:31, Robert Liao wrote:
> I would rather pay for the indentation here rather than to artificially break
up
> the array.
> 
> On 2013/08/08 01:15:10, vadimt wrote:
> > You could also add newline before ['notificationsData', 'recentDismissals'],
> > function(items) {
> > 
> > This way, you wouldn't have to shift the whole function's body.
> 

You can fit the whole array AND "function(items) {" in 1 new line.
But up to you.

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:873:
instrumented.runtime.onInstalled.addListener(function(details) {
On 2013/08/08 17:43:31, Robert Liao wrote:
> It appears so. Right after launch the event page is not in the inactive state.
> 
> I suspect it is keying off of our extension manifest to determine what to do.
> The background directive in the manifest is sufficient to do the initial load.
> 
> http://developer.chrome.com/extensions/event_pages.html
> 
> The event page is loaded when it is "needed", and unloaded when it goes idle
> again. Here are some examples of things that will cause the event page to
load:
> 
> The app or extension is first installed or is updated to a new version (in
order
> to register for events).
> The event page was listening for an event, and the event is dispatched.
> A content script or other extension sends a message.
> Another view in the extension (for example, a popup) calls
> runtime.getBackgroundPage.
> On 2013/08/08 01:15:10, vadimt wrote:
> > So, Chrome recognizes this call and loads the page on install with this
event?
> 

I've started a thread to clarify this.

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
File chrome/browser/resources/google_now/utility.js (left):

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/utility.js:348: var storage =
chrome.storage.local;
On 2013/08/08 17:43:31, Robert Liao wrote:
> instrumented.storage.local doesn't exist until after buildTaskManager.
> 
> We'd have to instantiate buildTaskManager here for this to work.
> On 2013/08/08 01:15:10, vadimt wrote:
> > Why not  var storage = instrumented.storage.local;
> > ?
> 

I's always a good idea to instrument stuff before the code that uses it. Treat
this as 'using' directive.
Let's instrument storage before the task manager, and define 'storage' variable
in the next line.

https://codereview.chromium.org/22647003/diff/10001/chrome/browser/resources/...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/22647003/diff/10001/chrome/browser/resources/...
chrome/browser/resources/google_now/utility.js:316:
instrumentedContainer[functionName] = function() {
Alternatively, you could simply return the new function from the method and add
it to the container outside. This way, you'd not need to pass
instrumentedContainer parameter.

But up to you.

https://codereview.chromium.org/22647003/diff/10001/chrome/browser/resources/...
chrome/browser/resources/google_now/utility.js:355: instrumentedContainer =
instrumentedContainer[fragment];
Or, if you prefer:
instrumentedContainer = instrumentedContainer[fragment] =
instrumentedContainer[fragment] || {};

[robliao, 2013-08-08 21:35:35]

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/background.js:254: function(items) {
The right thing to do here is to keep the separate.
On 2013/08/08 18:19:25, vadimt wrote:
> On 2013/08/08 17:43:31, Robert Liao wrote:
> > I would rather pay for the indentation here rather than to artificially
break
> up
> > the array.
> > 
> > On 2013/08/08 01:15:10, vadimt wrote:
> > > You could also add newline before ['notificationsData',
'recentDismissals'],
> > > function(items) {
> > > 
> > > This way, you wouldn't have to shift the whole function's body.
> > 
> 
> You can fit the whole array AND "function(items) {" in 1 new line.
> But up to you.

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
File chrome/browser/resources/google_now/utility.js (left):

https://codereview.chromium.org/22647003/diff/1/chrome/browser/resources/goog...
chrome/browser/resources/google_now/utility.js:348: var storage =
chrome.storage.local;
Per discussion, I am going to leave things as is. Having a direct
chrome.storage.local.set/get allows for the bifurcation of storage to
instrumented and noninstrumented parts as well as keeping the calls simple. 

On 2013/08/08 18:19:25, vadimt wrote:
> On 2013/08/08 17:43:31, Robert Liao wrote:
> > instrumented.storage.local doesn't exist until after buildTaskManager.
> > 
> > We'd have to instantiate buildTaskManager here for this to work.
> > On 2013/08/08 01:15:10, vadimt wrote:
> > > Why not  var storage = instrumented.storage.local;
> > > ?
> > 
> 
> I's always a good idea to instrument stuff before the code that uses it. Treat
> this as 'using' directive.
> Let's instrument storage before the task manager, and define 'storage'
variable
> in the next line.

https://codereview.chromium.org/22647003/diff/10001/chrome/browser/resources/...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/22647003/diff/10001/chrome/browser/resources/...
chrome/browser/resources/google_now/utility.js:316:
instrumentedContainer[functionName] = function() {
Sounds better.
On 2013/08/08 18:19:25, vadimt wrote:
> Alternatively, you could simply return the new function from the method and
add
> it to the container outside. This way, you'd not need to pass
> instrumentedContainer parameter.
> 
> But up to you.

https://codereview.chromium.org/22647003/diff/10001/chrome/browser/resources/...
chrome/browser/resources/google_now/utility.js:355: instrumentedContainer =
instrumentedContainer[fragment];
Keeping current for explicitness.
On 2013/08/08 18:19:25, vadimt wrote:
> Or, if you prefer:
> instrumentedContainer = instrumentedContainer[fragment] =
> instrumentedContainer[fragment] || {};

[vadimt, 2013-08-08 22:41:26]

lgtm

https://codereview.chromium.org/22647003/diff/15001/chrome/browser/resources/...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/22647003/diff/15001/chrome/browser/resources/...
chrome/browser/resources/google_now/utility.js:312: instrumentedContainer,
You don't need this param.

[robliao, 2013-08-08 22:55:07]

https://codereview.chromium.org/22647003/diff/15001/chrome/browser/resources/...
File chrome/browser/resources/google_now/utility.js (right):

https://codereview.chromium.org/22647003/diff/15001/chrome/browser/resources/...
chrome/browser/resources/google_now/utility.js:312: instrumentedContainer,
Indeed!
On 2013/08/08 22:41:27, vadimt wrote:
> You don't need this param.

[robliao, 2013-08-08 22:55:31]

xiyuan: Please provide owner approval for this CL.

Thanks!

[xiyuan, 2013-08-08 23:26:29]

lgtm

https://codereview.chromium.org/22647003/diff/20001/chrome/browser/resources/...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/22647003/diff/20001/chrome/browser/resources/...
chrome/browser/resources/google_now/background.js:837: });
nit: this should align with 'get' at line 823 since it's the closing part of the
function started there.

[robliao, 2013-08-09 00:44:52]

https://codereview.chromium.org/22647003/diff/20001/chrome/browser/resources/...
File chrome/browser/resources/google_now/background.js (right):

https://codereview.chromium.org/22647003/diff/20001/chrome/browser/resources/...
chrome/browser/resources/google_now/background.js:837: });
On 2013/08/08 23:26:29, xiyuan wrote:
> nit: this should align with 'get' at line 823 since it's the closing part of
the
> function started there.

Done.

[commit-bot: I haz the power, 2013-08-09 00:50:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robliao@chromium.org/22647003/26001

[commit-bot: I haz the power, 2013-08-09 01:57:07]

Retried try job too often on linux_chromeos for step(s) interactive_ui_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[commit-bot: I haz the power, 2013-08-09 17:07:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/robliao@chromium.org/22647003/26001

[commit-bot: I haz the power, 2013-08-09 17:27:24]

Change committed as 216713