Bug 38147

Summary: [chromium] WebKit API additions to support appcache in workers.
Product: WebKit Reporter: Michael Nordman <michaeln@google.com>
Component: WebKit Misc.Assignee: Michael Nordman <michaeln@google.com>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue@webkit.org, dglazkov@chromium.org, dimich@chromium.org, webkit.review.bot@gmail.com
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Mac OS X 10.5   
Attachments:
Description Flags
patch
commit-queue: commit‑queue-
added DRT/chromium mods
michaeln: review-
add some WebKit:: namespace prefixes
dimich: review-
take4
none
replace NULL with 0 none

Description From 2010-04-26 16:15:53 PST
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.
------- Comment #1 From 2010-04-26 16:50:46 PST -------
Created an attachment (id=54348) [details]
patch
------- Comment #2 From 2010-04-26 17:01:14 PST -------
(From update of attachment 54348 [details])
Rejecting patch 54348 from commit-queue.

michaeln@google.com 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 eseidel@chromium.org to request a commit-queue restart.  After restart the commit-queue will correctly respect your committer rights.
------- Comment #3 From 2010-04-26 17:33:13 PST -------
Attachment 54348 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1801129
------- Comment #4 From 2010-04-26 17:55:56 PST -------
(In reply to comment #3)
> Attachment 54348 [details] [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?)
------- Comment #5 From 2010-04-26 17:58:13 PST -------
Created an attachment (id=54358) [details]
added DRT/chromium mods
------- Comment #6 From 2010-04-26 18:10:00 PST -------
Created an attachment (id=54362) [details]
add some WebKit:: namespace prefixes
------- Comment #7 From 2010-04-27 16:27:45 PST -------
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?
------- Comment #8 From 2010-04-27 16:33:16 PST -------
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
------- Comment #9 From 2010-04-27 16:37:06 PST -------
(From update of attachment 54362 [details])
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.
------- Comment #10 From 2010-04-27 16:41:22 PST -------
(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?
------- Comment #11 From 2010-04-27 16:55:32 PST -------
(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.
------- Comment #12 From 2010-04-27 18:38:16 PST -------
Created an attachment (id=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.
------- Comment #13 From 2010-04-27 19:50:56 PST -------
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.
------- Comment #14 From 2010-04-27 19:57:59 PST -------
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.
------- Comment #15 From 2010-04-27 20:00:17 PST -------
Created an attachment (id=54509) [details]
replace NULL with 0
------- Comment #16 From 2010-04-27 20:03:50 PST -------
(From update of attachment 54509 [details])
r=me
------- Comment #17 From 2010-04-27 22:40:32 PST -------
(From update of attachment 54509 [details])
Clearing flags on attachment: 54509

Committed r58380: <http://trac.webkit.org/changeset/58380>
------- Comment #18 From 2010-04-27 22:40:38 PST -------
All reviewed patches have been landed.  Closing bug.