Bug 38147 – [chromium] WebKit API additions to support appcache in workers.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Michael Nordman]

This is a subtask of http://code.google.com/p/chromium/issues/detail?id=39368

Add two things to the webkit API.

1) WebURLRequest TargetTypes for worker and shared worker main resources.

2) Factory method on class WebCommonWorkerClient to createApplicationCacheHost() for the associated worker.

[Michael Nordman]

Created attachment 54348 [details]
patch

[WebKit Commit Bot]

Comment on attachment 54348 [details]
patch

Rejecting patch 54348 from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/common/config/committers.py.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in WebKitTools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed).  Due to bug 30084 the commit-queue will require a restart after your change.  Please contact [e-mail] to request a commit-queue restart.  After restart the commit-queue will correctly respect your committer rights.

[WebKit Review Bot]

Attachment 54348 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1801129

[Michael Nordman]

(In reply to comment #3)
> Attachment 54348 [details] did not build on chromium:
> Build output: http://webkit-commit-queue.appspot.com/results/1801129

I missed a derived class in DRT/chromium! New patch coming soon (this time with
commit-queue?)

[Michael Nordman]

Created attachment 54358 [details]
added DRT/chromium mods

[Michael Nordman]

Created attachment 54362 [details]
add some WebKit:: namespace prefixes

[Dmitry Titov]

Target types are defined bun not used in this patch. Can they be a part of some other patch, where they are actually used? Or are they sued entirely in the code that is not part of the WebKit repo?

[Michael Nordman]

I'm just touching the webkit API in this patch. I have a subsquent webcore patch that defines corresponding enum values in webcore and uses them. See http://codereview.chromium.org/1521038/show

[Dmitry Titov]

Comment on attachment 54362 [details]
add some WebKit:: namespace prefixes

And a small nit:

> Index: WebKit/chromium/src/WebWorkerBase.cpp

> +void WebWorkerBase::didCreateDataSource(WebFrame* frame, WebDataSource* ds)
> +{
> +    // Tell the loader to load the data into the 'shadow page' synchronously,
> +    // so we can grab the resulting Document right after load.
> +    static_cast<WebDataSourceImpl*>(ds)->setDeferMainResourceDataLoad(false);
> +}

'frame' is not used in this function, should be omitted.

Almost there.

[Dmitry Titov]

(In reply to comment #8)
> I'm just touching the webkit API in this patch. I have a subsquent webcore
> patch that defines corresponding enum values in webcore and uses them. See
> http://codereview.chromium.org/1521038/show

So we have 2 enums that should be kept in sync. Perhaps making the change in the same patch could be better, to keep changes to related things together?

[Michael Nordman]

(In reply to comment #10)
> (In reply to comment #8)
> > I'm just touching the webkit API in this patch. I have a subsquent webcore
> > patch that defines corresponding enum values in webcore and uses them. See
> > http://codereview.chromium.org/1521038/show
> 
> So we have 2 enums that should be kept in sync. Perhaps making the change in
> the same patch could be better, to keep changes to related things together?

We could smush them together. I had them broken up for a few reasons.

1) I thought we liked smaller patches. If i put them together i could just as easily hear "maybe you should separate the API from the webcore changes" which sounds just as valid as "please smush them together".

2) Chrome has compile time dependencies on the API. Once landed, its in and I can code to it in chrome. If smushed together, there's a chance that this will hiccup somewhere and get rolled out. If the API changes are rolled out after chrome depends on it, that could break the Chrome build.
 
3) Need to run the webcore changes thru webkit tests and that's easier said than done on windows.

I went with two patches for those reasons. The one that bugs me the most is the second item.

[Michael Nordman]

Created attachment 54494 [details]
take4

Removed the unused param name per dimich's comment. Also added a call to frame->setClient(NULL) in the dtor since we're no longer using a global frameClient instance.

[WebKit Review Bot]

Attachment 54494 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebKit/chromium/src/WebWorkerBase.cpp:84:  Use 0 instead of NULL.  [readability/null] [5]
Total errors found: 1 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Nordman]

Given that frames are refcounted objects with 'squishy' lifetimes. I'd be more comfortable with an explicit call to setClient(NULL) at WebWorkerBase dtor time. I just stepped thru in the debugger to see the sequence of events. Under normal circumstances this definitely isn't needed... but since 'frames' are refcounted there may be cases where it actually outlives WebWorkerBase.

I'll post the webcore changes in a second patch for review coincident with this one. I'd like to keep them in different patches for the reasons mentioned earlier.

[Michael Nordman]

Created attachment 54509 [details]
replace NULL with 0

[Dmitry Titov]

Comment on attachment 54509 [details]
replace NULL with 0

r=me

[WebKit Commit Bot]

Comment on attachment 54509 [details]
replace NULL with 0

Clearing flags on attachment: 54509

Committed r58380: <http://trac.webkit.org/changeset/58380>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.