Bug 26041 – Allow adding resource source to frame asynchronously

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Yury Semikhatsky]

Chromium's async/out-of-process version of WebInspector needs to add resource source to WebInspector.SourceView asynchronously since the source is retrieved from the inspected process asynchronously.

Current implementation of InspectorController::addResourceSourceToFrame supports only synchronous execution. The idea is to change it into asynchronous method that invokes a callback when the source is added to notify the caller.

[Yury Semikhatsky]

Created attachment 30709 [details]
Change addResourceSourceToFrame so that it invokes callback when the source is loaded.

[Yury Semikhatsky]

(In reply to comment #1)
> Created an attachment (id=30709) [review]
> Change addResourceSourceToFrame so that it invokes callback when the source is
> loaded.
> 

The patch changes WebInspector front-end so that it expects InspectorController.addResourceSourceToFrame to add the script source asynchronously. When the source is loaded the front-end is notified by call to InspectorFrontend::didAddResourceSourceToFrame which will execute SourceView.addResourceSourceToFrameCallback for the resource to run all delayed actions including syntax highlight, reveal and highlight line. 

Custom implementations of addSourceToFrame and addResourceSourceToFrame can now be merged into a common one. 

There is an issue with resource identifier passed as long value to InspectorController.addResourceSourceToFrame.  A short introduction: all resources loaded not from cache will have identifiers of type 'unsigned long'. Resources loaded from memory cache will have no identifiers. To work with both types of resources uniformly InspecorController.didLoadResourceFromMemoryCache assigns some surrogate identifiers to the cached resources. The identifiers are always negative to guarantee that they don't clash with existing ones. To fit both types of identifiers InspectorResource.m_identifier has type 'long long'. However JavaScript Number cannot fit all 'long long' values that is why in InspectorController.idl I couldn't declare  addResourceSourceToFrame as accepting 'long long indetifier'. 'double' type would be big enough to fit all the values but it looks ugly when an argument with name 'identifier' has 'double' type. In C++ code it can be mitigated by using 'typdef double ResourceId' but we would still need to decalre it as double in the .idl  In the current version the ids are passed as 'long' to the javascript code which may lead to loss of accuracy. Any thoughts on that?

[Timothy Hatcher]

Why can't we just use a load event listener on the iframe? I am pretty sure we do that already in one place.

[Yury Semikhatsky]

Created attachment 30732 [details]
Complete source frame loading in iframe 'load' event listener.

[Yury Semikhatsky]

(In reply to comment #3)
> Why can't we just use a load event listener on the iframe? I am pretty sure we
> do that already in one place.
> 
Done. I've added 'content loaded' event to SourceFrame so that it has a chance to perform its own 'load' event handling when iframe is loaded before its clients are notified. 

[Timothy Hatcher]

Comment on attachment 30732 [details]
Complete source frame loading in iframe 'load' event listener.

Seems odd addSourceToFrame returns a bool and addResourceSourceToFrame does not. Can addResourceSourceToFrame still return a bool? If the resource isn't found retuen false and retuen the result of addSourceToFrame.

+        delete this._frameNeedsSetup;

You shouldn't need this sine _frameNeedsSetup was deleted in setupSourceFrameIfNeeded.

[Yury Semikhatsky]

(In reply to comment #6)
> (From update of attachment 30732 [details] [review])
> Seems odd addSourceToFrame returns a bool and addResourceSourceToFrame does
> not. Can addResourceSourceToFrame still return a bool? If the resource isn't
> found retuen false and retuen the result of addSourceToFrame.
> 
addSourceToFrame always work sinchronously because all data it may need are passed in the arguments while addResourceSourceToFrame in case of Chrome may need to request script source for given identifier from the inspected process. Until response is received we cannot tell whether the source adding succeeded or not. That is the reason why addResourceSourceToFrame doesn't return 'bool' anymore. Instead if the source is added successfuly the iframe will generate 'load' event.



> +        delete this._frameNeedsSetup;
> 
> You shouldn't need this sine _frameNeedsSetup was deleted in
> setupSourceFrameIfNeeded.
> 
In the asynchronous case the following scenario is possible:
- setupSourceFrameIfNeeded calls addResourceSourceToFrame which sends request for the source and returns
- _resourceLoadingFinished is called and sets this._frameNeedsSetup to true
- response to the source request is received, content is loaded and _frameNeedsSetup is not cleared which causes one extra frame loading in the next call to setupSourceFrameIfNeeded

[Dimitri Glazkov (Google)]

I'll land once Tim agrees/disagrees w/Yuri's last comment :)

[Timothy Hatcher]

Yury, makes sense. This is good to land.

[Dimitri Glazkov (Google)]

Landed as http://trac.webkit.org/changeset/44230.

Yury, in the future patches, don't forget to:

* set your EMAIL_ADDRESS environment variable
* add URL of the bug to the ChangeLog entry.

[Yury Semikhatsky]

(In reply to comment #10)
> Landed as http://trac.webkit.org/changeset/44230.
> 
> Yury, in the future patches, don't forget to:
> 
> * set your EMAIL_ADDRESS environment variable
> * add URL of the bug to the ChangeLog entry.
> 
Sorry about that.