Bug 38147 - [chromium] WebKit API additions to support appcache in workers.
Summary: [chromium] WebKit API additions to support appcache in workers.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Michael Nordman
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-04-26 16:15 PDT by Michael Nordman
Modified: 2010-04-27 22:40 PDT (History)
4 users (show)

See Also:


Attachments
patch (7.12 KB, patch)
2010-04-26 16:50 PDT, Michael Nordman
commit-queue: commit-queue-
Details | Formatted Diff | Diff
added DRT/chromium mods (8.51 KB, patch)
2010-04-26 17:58 PDT, Michael Nordman
michaeln: review-
Details | Formatted Diff | Diff
add some WebKit:: namespace prefixes (8.53 KB, patch)
2010-04-26 18:10 PDT, Michael Nordman
dimich: review-
Details | Formatted Diff | Diff
take4 (8.80 KB, patch)
2010-04-27 18:38 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff
replace NULL with 0 (8.80 KB, patch)
2010-04-27 20:00 PDT, Michael Nordman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Nordman 2010-04-26 16:15:53 PDT
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 Michael Nordman 2010-04-26 16:50:46 PDT
Created attachment 54348 [details]
patch
Comment 2 WebKit Commit Bot 2010-04-26 17:01:14 PDT
Comment on attachment 54348 [details]
patch

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 WebKit Review Bot 2010-04-26 17:33:13 PDT
Attachment 54348 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1801129
Comment 4 Michael Nordman 2010-04-26 17:55:56 PDT
(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?)
Comment 5 Michael Nordman 2010-04-26 17:58:13 PDT
Created attachment 54358 [details]
added DRT/chromium mods
Comment 6 Michael Nordman 2010-04-26 18:10:00 PDT
Created attachment 54362 [details]
add some WebKit:: namespace prefixes
Comment 7 Dmitry Titov 2010-04-27 16:27:45 PDT
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 Michael Nordman 2010-04-27 16:33:16 PDT
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 Dmitry Titov 2010-04-27 16:37:06 PDT
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.
Comment 10 Dmitry Titov 2010-04-27 16:41:22 PDT
(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 Michael Nordman 2010-04-27 16:55:32 PDT
(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 Michael Nordman 2010-04-27 18:38:16 PDT
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.
Comment 13 WebKit Review Bot 2010-04-27 19:50:56 PDT
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 Michael Nordman 2010-04-27 19:57:59 PDT
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 Michael Nordman 2010-04-27 20:00:17 PDT
Created attachment 54509 [details]
replace NULL with 0
Comment 16 Dmitry Titov 2010-04-27 20:03:50 PDT
Comment on attachment 54509 [details]
replace NULL with 0

r=me
Comment 17 WebKit Commit Bot 2010-04-27 22:40:32 PDT
Comment on attachment 54509 [details]
replace NULL with 0

Clearing flags on attachment: 54509

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