For local review prior to sending to webkit

For local review prior to sending to webkit.Please see https://bugs.webkit.org/show_bug.cgi?id=254361)1) This patch is mostly about allowing development of a new appcache system without disrupting the existing system. It introduces a new ENABLE flag for the new system, ENABLE(APPLICATION_CACHE), while leaving the flag for the existing system in place, ENABLE(OFFLINE_WEB_APPLICATIONS). There is some overlap between the two mostly around the window.applicationCache attribute.2) Also defines an interface to the new system for use in the frontend, class ApplicationCacheFrontend. The interface is being used by FrameLoader and DOMApplicationCache and HTMLHtmlElement. There is only a stub implementation of this interface at this time that does no harm.3) Also widens the ResourceRequestBase and ResourceResponseBase data structures with additional data members to provide additional inputs (and outputs) to (from) the backend when loading a ResourceHandle.4) Requisite custom V8Bindings for the DOMApplicationCache interface. Most of the bindings work isn't destined for the webkit repository just yet, that stuff is still being migrated over in general.5) Changes to the JSBindings for the DOMApplicationCache interface to bind to the new or existing system based on which ENABLE flag is set.

[michaeln, 2009-05-18 21:53:52]

Hi Guys, before I send this patch for review to the webkit, wanted tog bounce it
off you all.

[jorlow, 2009-05-18 23:28:38]

Many of my comments apply to more than one part of the patch, so when I point
out an issue think about whether it applies elsewhere.

Also, in the future, even 'preview' patches should have better descriptions so
it's easier to read.

Lastly, be sure to mention in the description what files are 'svn cp'ed because
it's not obvious just looking at the files in patch form.

http://codereview.chromium.org/113554/diff/1/14
File bindings/v8/custom/V8DOMApplicationCacheCustom.cpp (right):

http://codereview.chromium.org/113554/diff/1/14#newcode34
Line 34: #if ENABLE(APPLICATION_CACHE)
I think the normal thing to do is not ifdef an entire file.  WebCore's
GNUMakefile and I assume the other project files support things being compiled
based on flags present.  This is what's done for DOM_STORAGE at least.

http://codereview.chromium.org/113554/diff/1/14#newcode44
Line 44: // Things implemented in here...
Delete this stuff.

http://codereview.chromium.org/113554/diff/1/14#newcode111
Line 111: DEFINE_APPCACHE_EVENT_HANDLER_GETTER(updateready)
Should this be somewhere else?  IIRC v8_custom.h or v8_proxy.h is the usual
place for this stuff, right?

http://codereview.chromium.org/113554/diff/1/14#newcode131
Line 131: DOMApplicationCache* appcache =
V8Proxy::ToNativeObject<DOMApplicationCache>(V8ClassIndex::DOMAPPLICATIONCACHE,
args.Holder());
Are you positive you need all of this stuff?  After a quick look through, I
don't see anything that can't be accomplished with standard event handling
stuff.

http://codereview.chromium.org/113554/diff/1/24
File loader/ApplicationCacheScripting.cpp (right):

http://codereview.chromium.org/113554/diff/1/24#newcode41
Line 41: void ApplicationCacheScripting::selectCacheWithoutManifest(Frame*
frame) {
I think this would be much less confusing if you had ifdefs in each function
like

void fun() {
#if one way
   // do stuff
#elif //other wya
  // do old stuff
#endif
}

...if you want to leave the structure as is, I think you should split these into
2 files.

http://codereview.chromium.org/113554/diff/1/25
File loader/ApplicationCacheScripting.h (right):

http://codereview.chromium.org/113554/diff/1/25#newcode34
Line 34: // This file is temporary until the new application cache system is
brought up.
Is the whole file temporary, or just this part?  Looks to me only this part.

http://codereview.chromium.org/113554/diff/1/23
File loader/FrameLoader.cpp (right):

http://codereview.chromium.org/113554/diff/1/23#newcode323
Line 323: ApplicationCacheContextType contextType = MainFrameContext;
Needs to be ifdefed

Note that you definitely need to test this with safari before you send it for
review there.

http://codereview.chromium.org/113554/diff/1/23#newcode325
Line 325: if (m_frame->parentFrame()) {
Since you use this again, maybe it makes sense to put it in a temp variable. 
It's of course mostly for clarity since the compiler should be smart enough to
not expend extra effort.

http://codereview.chromium.org/113554/diff/1/23#newcode5328
Line 5328: // It's a foreigh entry, restart the current navigation from the top
of the navigation algorithm.
foreign?

http://codereview.chromium.org/113554/diff/1/26
File loader/FrameLoader.h (right):

http://codereview.chromium.org/113554/diff/1/26#newcode651
Line 651: OwnPtr<ApplicationCacheFrontend> m_appcacheFrontend;
What is the difference between an OwnPtr and a RefPtr?

http://codereview.chromium.org/113554/diff/1/17
File loader/appcache2/ApplicationCacheCommon.h (right):

http://codereview.chromium.org/113554/diff/1/17#newcode40
Line 40: typedef long long ApplicationCacheStorageID;  // a 64 bit sqlite rowid
Is "long long" the _exact_ type that SQLite uses?  If so, then this is probably
ok, otherwise you probably want to be using the webkit typedefs. (IIRC, int64?)

http://codereview.chromium.org/113554/diff/1/18
File loader/appcache2/ApplicationCacheFrontend.cpp (right):

http://codereview.chromium.org/113554/diff/1/18#newcode38
Line 38: namespace {
Why do you use an anonymous namespace like this?  IIRC, this is against one of
the style guides in most cases.

http://codereview.chromium.org/113554/diff/1/18#newcode45
Line 45: {
Need an assert to be sure this is only occurring on one thread.  (I don't
believe there's any x86 atomicity guarantees about ++, especially when using the
value it returns...and this code need to work for other processors.  If this
needs to work on multiple threads, can you use an atomic increment or compare
and swap/test and set loop?)

http://codereview.chromium.org/113554/diff/1/19
File loader/appcache2/DOMApplicationCache.cpp (right):

http://codereview.chromium.org/113554/diff/1/19#newcode97
Line 97: return cache->swapCache();
wait...swapCache() is purely virtual in ApplicationCacheFrontend....so does this
code work at all in its current form?

http://codereview.chromium.org/113554/diff/1/19#newcode103
Line 103: ec = INVALID_STATE_ERR;
set it to 0 if this isn't hit?

http://codereview.chromium.org/113554/diff/1/22
File loader/appcache2/DOMApplicationCache.idl (left):

http://codereview.chromium.org/113554/diff/1/22#oldcode46
Line 46: #if defined(ENABLE_APPLICATION_CACHE_DYNAMIC_ENTRIES) &&
ENABLE_APPLICATION_CACHE_DYNAMIC_ENTRIES
wait..what's this?

http://codereview.chromium.org/113554/diff/1/22
File loader/appcache2/DOMApplicationCache.idl (right):

http://codereview.chromium.org/113554/diff/1/22#newcode29
Line 29: Conditional=APPLICATION_CACHE,
I assume this was 'svn cp'ed?

http://codereview.chromium.org/113554/diff/1/8
File page/DOMWindow.cpp (right):

http://codereview.chromium.org/113554/diff/1/8#newcode79
Line 79: #if APPLICATION_CACHE_SCRIPTING_ENABLED
Use the ENABLE macros.

http://codereview.chromium.org/113554/diff/1/8#newcode438
Line 438: #if APPLICATION_CACHE_SCRIPTING_ENABLED
ditto

http://codereview.chromium.org/113554/diff/1/8#newcode508
Line 508: #if APPLICATION_CACHE_SCRIPTING_ENABLED
gonna stop mentioning it now...please make sure you take care of this everywhere

[dglazkov, 2009-05-19 18:43:15]

I apologize for not having more thorough comments, I am still trying to wrap my
head around this CL. Just some preliminary opinions:

http://codereview.chromium.org/113554/diff/1/14
File bindings/v8/custom/V8DOMApplicationCacheCustom.cpp (right):

http://codereview.chromium.org/113554/diff/1/14#newcode127
Line 127: 
These macros make me sad.

I really think we should just go with a generic accessor getter/setter, and then
routing the event based on the name supplied, like it's done in V8ElementCustom.

http://codereview.chromium.org/113554/diff/1/2
File dom/EventTarget.h (right):

http://codereview.chromium.org/113554/diff/1/2#newcode63
Line 63: #if APPLICATION_CACHE_SCRIPTING_ENABLED
This should be ENABLE(APPLICATION_CACHE_SCRIPTING).

http://codereview.chromium.org/113554/diff/1/24
File loader/ApplicationCacheScripting.cpp (right):

http://codereview.chromium.org/113554/diff/1/24#newcode41
Line 41: void ApplicationCacheScripting::selectCacheWithoutManifest(Frame*
frame) {
+1 on Jeremy's comment.

http://codereview.chromium.org/113554/diff/1/18
File loader/appcache2/ApplicationCacheFrontend.cpp (right):

http://codereview.chromium.org/113554/diff/1/18#newcode76
Line 76: ApplicationCacheFrontend::FactoryProc factory_proc;
factoryProc

[michaeln, 2009-05-19 19:22:08]

http://codereview.chromium.org/113554/diff/1/14
File bindings/v8/custom/V8DOMApplicationCacheCustom.cpp (right):

http://codereview.chromium.org/113554/diff/1/14#newcode34
Line 34: #if ENABLE(APPLICATION_CACHE)
Don't think so. Look at workers or database.

http://codereview.chromium.org/113554/diff/1/14#newcode44
Line 44: // Things implemented in here...
oh... i'll be deleting these comments

http://codereview.chromium.org/113554/diff/1/14#newcode44
Line 44: // Things implemented in here...
On 2009/05/18 23:28:39, Jeremy Orlow wrote:
> Delete this stuff.

Done.

http://codereview.chromium.org/113554/diff/1/14#newcode111
Line 111: DEFINE_APPCACHE_EVENT_HANDLER_GETTER(updateready)
This is where this should be as far as i can tell.

Usually these sort of event handlers are coded individually in these custom
bindings cpp files (see V8XMLHttpRequest for example). I wasn't too keen on
that, so I made the local macro for this purpose.

http://codereview.chromium.org/113554/diff/1/14#newcode127
Line 127: 
What's here now is based on what I see having been done for XHR, Workers, etc...

If there is some new and improved way of doing these requisite glopware
bindings... great... point me at an example of some class using that technique.

http://codereview.chromium.org/113554/diff/1/14#newcode131
Line 131: DOMApplicationCache* appcache =
V8Proxy::ToNativeObject<DOMApplicationCache>(V8ClassIndex::DOMAPPLICATIONCACHE,
args.Holder());
AFAIK, yes... if thats not true... i'm all ears.

http://codereview.chromium.org/113554/diff/1/24
File loader/ApplicationCacheScripting.cpp (right):

http://codereview.chromium.org/113554/diff/1/24#newcode41
Line 41: void ApplicationCacheScripting::selectCacheWithoutManifest(Frame*
frame) {
Fine grained blocks are easier to grok... ok.

This file has been deleted, see HTMLHtmlElement for the fine grained blocks.

http://codereview.chromium.org/113554/diff/1/25
File loader/ApplicationCacheScripting.h (right):

http://codereview.chromium.org/113554/diff/1/25#newcode34
Line 34: // This file is temporary until the new application cache system is
brought up.
Very temporary... deleted already :)

http://codereview.chromium.org/113554/diff/1/23
File loader/FrameLoader.cpp (right):

http://codereview.chromium.org/113554/diff/1/23#newcode323
Line 323: ApplicationCacheContextType contextType = MainFrameContext;
On 2009/05/18 23:28:39, Jeremy Orlow wrote:
> Needs to be ifdefed
> 
> Note that you definitely need to test this with safari before you send it for
> review there.

Done.

http://codereview.chromium.org/113554/diff/1/23#newcode325
Line 325: if (m_frame->parentFrame()) {
On 2009/05/18 23:28:39, Jeremy Orlow wrote:
> Since you use this again, maybe it makes sense to put it in a temp variable. 
> It's of course mostly for clarity since the compiler should be smart enough to
> not expend extra effort.

Done.

http://codereview.chromium.org/113554/diff/1/23#newcode5328
Line 5328: // It's a foreigh entry, restart the current navigation from the top
of the navigation algorithm.
On 2009/05/18 23:28:39, Jeremy Orlow wrote:
> foreign?

Done.

http://codereview.chromium.org/113554/diff/1/26
File loader/FrameLoader.h (right):

http://codereview.chromium.org/113554/diff/1/26#newcode651
Line 651: OwnPtr<ApplicationCacheFrontend> m_appcacheFrontend;
On 2009/05/18 23:28:39, Jeremy Orlow wrote:
> What is the difference between an OwnPtr and a RefPtr?

scoped_ptr<T> vs scoped_refptr<T>

http://codereview.chromium.org/113554/diff/1/17
File loader/appcache2/ApplicationCacheCommon.h (right):

http://codereview.chromium.org/113554/diff/1/17#newcode40
Line 40: typedef long long ApplicationCacheStorageID;  // a 64 bit sqlite rowid
On 2009/05/18 23:28:39, Jeremy Orlow wrote:
> Is "long long" the _exact_ type that SQLite uses?  If so, then this is
probably
> ok, otherwise you probably want to be using the webkit typedefs. (IIRC,
int64?)

Unclear to me what webkit uses for large integers... i've seen both long long
and int64_t used here and there. Either works for me. I chose long long here
since the compiler supports that primitive and there is no .h file involved.

http://codereview.chromium.org/113554/diff/1/18
File loader/appcache2/ApplicationCacheFrontend.cpp (right):

http://codereview.chromium.org/113554/diff/1/18#newcode38
Line 38: namespace {
On 2009/05/18 23:28:39, Jeremy Orlow wrote:
> Why do you use an anonymous namespace like this?  IIRC, this is against one of
> the style guides in most cases.

To hide this class and global variable from the rest of the world.  Certainly
not a violation of the google style guide. I'm not familiar enough with the
webkit style guide to know, although i do see other anon namespace in the
webcore code base.

I have to read the webkit style guide more closely.

http://codereview.chromium.org/113554/diff/1/18#newcode45
Line 45: {
On 2009/05/18 23:28:39, Jeremy Orlow wrote:
> Need an assert to be sure this is only occurring on one thread.  (I don't
> believe there's any x86 atomicity guarantees about ++, especially when using
the
> value it returns...and this code need to work for other processors.  If this
> needs to work on multiple threads, can you use an atomic increment or compare
> and swap/test and set loop?)

Not needed. The frontend is all single threaded. That may change when it comes
time to integrate with workers, but that time is not yet.

http://codereview.chromium.org/113554/diff/1/18#newcode76
Line 76: ApplicationCacheFrontend::FactoryProc factory_proc;
On 2009/05/19 18:43:15, Dimitri Glazkov wrote:
> factoryProc

Done.

http://codereview.chromium.org/113554/diff/1/19
File loader/appcache2/DOMApplicationCache.cpp (right):

http://codereview.chromium.org/113554/diff/1/19#newcode45
Line 45: appcacheFrontend()->setDelegate(this);
i should probably be consistent about the naming here,
applicationCacheFrontend() or appcacheFrontend() in both DOMApplicationCache and
FrameLoaer... which is more webkit friendly?

http://codereview.chromium.org/113554/diff/1/19#newcode97
Line 97: return cache->swapCache();
On 2009/05/18 23:28:39, Jeremy Orlow wrote:
> wait...swapCache() is purely virtual in ApplicationCacheFrontend....so does
this
> code work at all in its current form?

There is a concrete impl provide for this initial checking of these interfaces,
class StubFrontend.

http://codereview.chromium.org/113554/diff/1/19#newcode103
Line 103: ec = INVALID_STATE_ERR;
On 2009/05/18 23:28:39, Jeremy Orlow wrote:
> set it to 0 if this isn't hit?

Isn't required and isn't generally done from what i can see.

http://codereview.chromium.org/113554/diff/1/22
File loader/appcache2/DOMApplicationCache.idl (left):

http://codereview.chromium.org/113554/diff/1/22#oldcode46
Line 46: #if defined(ENABLE_APPLICATION_CACHE_DYNAMIC_ENTRIES) &&
ENABLE_APPLICATION_CACHE_DYNAMIC_ENTRIES
On 2009/05/18 23:28:39, Jeremy Orlow wrote:
> wait..what's this?

API not in the HTML5 spec. I'm not planning to support this initially at least.

http://codereview.chromium.org/113554/diff/1/22
File loader/appcache2/DOMApplicationCache.idl (right):

http://codereview.chromium.org/113554/diff/1/22#newcode29
Line 29: Conditional=APPLICATION_CACHE,
On 2009/05/18 23:28:39, Jeremy Orlow wrote:
> I assume this was 'svn cp'ed?

Yes DOMApplicationCache.* was copied and edited to taste.

http://codereview.chromium.org/113554/diff/1/8
File page/DOMWindow.cpp (right):

http://codereview.chromium.org/113554/diff/1/8#newcode79
Line 79: #if APPLICATION_CACHE_SCRIPTING_ENABLED
On 2009/05/18 23:28:39, Jeremy Orlow wrote:
> Use the ENABLE macros.

Done.

Good idea, i've switched to using ENABLE(APPLICATION_CACHE) ||
ENABLE(OFFLINE_WEB_APPLICATIONS) at these sites, and remove the awkward
ApplicationCacheScript.h .cpp file altogether.

[michaeln, 2009-05-19 19:32:04]

Ah... i see a source of confusion around the v8 bindings.

Very little of that shows up in webcore proper. For whatever reason, the custom
bindings .cpp file does, but most of it does not yet show up there.

I'll put up another CL with the changes to our glue/port in src/webkit (as
opposed the third_party/WebKit)

[michaeln, 2009-05-19 20:18:06]

See http://codereview.chromium.org/115531 for the bindings in chromium's svn

[jorlow, 2009-05-19 20:46:42]

Here are my comments on your comments from the first patch.

Will try to look at the new one within 24 hours.

http://codereview.chromium.org/113554/diff/1/14
File bindings/v8/custom/V8DOMApplicationCacheCustom.cpp (right):

http://codereview.chromium.org/113554/diff/1/14#newcode34
Line 34: #if ENABLE(APPLICATION_CACHE)
On 2009/05/19 19:22:09, michaeln wrote:
> Don't think so. Look at workers or database.

Database does not.  Workers does, but most stuff doesn't.

Seems pretty inefficient to "compile" files that don't actually have code in
them.

http://codereview.chromium.org/113554/diff/1/14#newcode111
Line 111: DEFINE_APPCACHE_EVENT_HANDLER_GETTER(updateready)
On 2009/05/19 19:22:09, michaeln wrote:
> This is where this should be as far as i can tell.
> 
> Usually these sort of event handlers are coded individually in these custom
> bindings cpp files (see V8XMLHttpRequest for example). I wasn't too keen on
> that, so I made the local macro for this purpose.
> 

I still don't understand why you can't just use idl generated code for this. 
It'd work a bit differently, but as far as I can tell you don't need all this
custom stuff.

Take a look at some of the other event handling code for standard events.

http://codereview.chromium.org/113554/diff/1/18
File loader/appcache2/ApplicationCacheFrontend.cpp (right):

http://codereview.chromium.org/113554/diff/1/18#newcode45
Line 45: {
On 2009/05/19 19:22:09, michaeln wrote:
> On 2009/05/18 23:28:39, Jeremy Orlow wrote:
> > Need an assert to be sure this is only occurring on one thread.  (I don't
> > believe there's any x86 atomicity guarantees about ++, especially when using
> the
> > value it returns...and this code need to work for other processors.  If this
> > needs to work on multiple threads, can you use an atomic increment or
compare
> > and swap/test and set loop?)
> 
> Not needed. The frontend is all single threaded. That may change when it comes
> time to integrate with workers, but that time is not yet.

If it's all single threaded, please use asserts.  If nothing else, it documents
the assumptions.

We may need to come up with our own special macro for stuff running within
Chromium other than just asserting we're in the main thread (since what the main
thread is unclear there).  Maybe something like:

#defined CHECK_THREAD( var ) do { if (UNLIKELY(!var)) var = CurrentThread();
ASSERT(var == CurrentThread(); } while (0)

And then use it like.

Thread* appCacheBackendThread = 0;  // global

and later

void someFunction() {
  CHECK_THREAD(appCacheBackendThread);
  // ...
}

Just a thought.

[michaeln, 2009-05-19 21:04:39]

> I still don't understand why you can't just use idl generated code for this. 
> It'd work a bit differently, but as far as I can tell you don't need all this
> custom stuff.
>
> Take a look at some of the other event handling code for standard events.

As mentioned before, what I've done here is based on what I've seen done for
many other event handlers (see XMLHttpRequestOnreadystatechange for example).
Can you show me a specific example an event handler treated in the way you're
referring to?

[michaeln, 2009-05-20 01:46:12]

In hindsight, it was probably a mistake to include the V8Bindings in this CL. 
I'm much more interested in getting the webcore changes looked at. The gory
details of the v8 bindings are a distraction. I won't make that mistake when
sending for review to to webkit.

Having said that, to close on the bindings discussion. After talking to mike and
dimitry, I think the code in this CL represents the current state of the v8
bindings art given the infrastructure at the moment. With some changes to the
code generator (or perhaps some changes to the DOMApplicationCache.idl), the
bindings code could reduced. I think these things are out of scope for this
webcore CL.

Back to the real webcore changes...

* Made changes to JSDOMApplicationCacheCustom.cpp to support the new ENABLE flag
too.

* Added a new method to the ApplicationCacheFrontend for use in FrameLoader when
determining if the current page can be added to the page cache. (this was a TODO
before).

* I don't think the assert(isMainFrame()) test is warranted in the StubFrontend
class. This really is a temporary stub class for one. And webcore code
overwhelmingly does not make such assertions, this code is no different.

* I have looked thru the coding style guide, I think i'm complying with it, but
probably there are places i've missed on that front.

Please take another look at the webcore code. I'll probably be sending this up
to webkit soonish (tomorrow or the next day).

[michaeln, 2009-05-21 01:15:28]

New snapshot uploaded.

Now with slimmer and trimmer custom bindings for the event handlers. One
getter/setter pair for all 8 handlers, with the event name being a parameter. A
little step closer to being 100% generated.

(see http://codereview.chromium.org/115531 for related changes to the code
generator)

[michaeln, 2009-05-22 18:35:53]

New snapshot uploaded.

Found a couple more files in JSBindings that need to respect the
ENABLE(APPLICATION_CACHE) flag.

[michaeln, 2009-05-27 02:32:11]

New snapshot uploaded per recent discussions on webkit-dev list frowning on
virtual interfaces. Made ApplicationCacheFrontend a concrete class.

[dglazkov, 2009-05-27 16:30:34]

I am digging this :)

I am sure you're aware of the nomenclature brew up on webkit-dev in regard to
Frontend/Backend.

Few comments below:

http://codereview.chromium.org/113554/diff/251/264
File bindings/v8/custom/V8DOMApplicationCacheCustom.cpp (right):

http://codereview.chromium.org/113554/diff/251/264#newcode63
Line 63: return PassRefPtr<EventListener>();
Can just do return 0.

http://codereview.chromium.org/113554/diff/251/264#newcode67
Line 67: {
can just shorten this to "toV8".

http://codereview.chromium.org/113554/diff/251/266
File html/HTMLHtmlElement.cpp (right):

http://codereview.chromium.org/113554/diff/251/266#newcode33
Line 33: #if ENABLE(OFFLINE_WEB_APPLICATIONS)
I think you should put a FIXME here stating why we have to do this temporarily
and your eventual intent to get rid of these.

http://codereview.chromium.org/113554/diff/251/273
File loader/FrameLoader.cpp (right):

http://codereview.chromium.org/113554/diff/251/273#newcode5327
Line 5327: 
This can be just one line :) and brace on new line.

http://codereview.chromium.org/113554/diff/251/270
File loader/appcache2/ApplicationCacheFrontend.h (right):

http://codereview.chromium.org/113554/diff/251/270#newcode94
Line 94: class Delegate {
I think WebKit convention is to name these Client and methods start with Did, as
in DidCheck, DidReportError, etc.

Also, typically they are not nested classes.

http://codereview.chromium.org/113554/diff/251/270#newcode109
Line 109: void setDelegate(Delegate* delegate) { m_delegate = delegate; }
setClient, similarly.

http://codereview.chromium.org/113554/diff/251/271
File loader/appcache2/DOMApplicationCache.h (right):

http://codereview.chromium.org/113554/diff/251/271#newcode115
Line 115: #if USE(V8)
I think it is a bad idea to introduce script-engine-specific ifdefs here,
especially considering that JSC doesn't need them. Can we just mimic what JSC
does here?

http://codereview.chromium.org/113554/diff/251/271#newcode151
Line 151: // Returns a pointer to the appropiate data member above
Ditto. A RefPtr<Anything>* just looks plain weird.

http://codereview.chromium.org/113554/diff/251/272
File loader/appcache2/DOMApplicationCache.idl (left):

http://codereview.chromium.org/113554/diff/251/272#oldcode46
Line 46: #if defined(ENABLE_APPLICATION_CACHE_DYNAMIC_ENTRIES) &&
ENABLE_APPLICATION_CACHE_DYNAMIC_ENTRIES
Since you dropped ENABLE_APPLICATION_CACHE_DYNAMIC_ENTRIES here, perhaps that
whole guard is not needed anywhere?

http://codereview.chromium.org/113554/diff/251/260
File platform/network/ResourceResponseBase.h (right):

http://codereview.chromium.org/113554/diff/251/260#newcode172
Line 172: #endif
It seems weird that we have both the ID and the manifest URL here. Why do we
need that?

Also, this may be a contentious issue -- adding to the size of the
ResourceResponseBase.

If you do need both of them, perhaps these should be something like an
SuperDuperAughmentedAppCacheID class, which includes manifest URL and the ID?

[michaeln, 2009-05-27 21:05:24]

Oh... just now noticed your comments!

Between then and now, I've uploaded this patch for review at webkit. I'll take a
look at your comments and do something appropiate with them of course... but
would you also submit these comments there so that when i upload an updated
patch there the webkit community knows why?

(gotta love two review tools and two repositories btw:)

[michaeln, 2009-05-27 21:55:00]

http://codereview.chromium.org/113554/diff/251/264
File bindings/v8/custom/V8DOMApplicationCacheCustom.cpp (right):

http://codereview.chromium.org/113554/diff/251/264#newcode63
Line 63: return PassRefPtr<EventListener>();
Done... good to know, thanx

http://codereview.chromium.org/113554/diff/251/264#newcode67
Line 67: {
toV8() sounds very generic, these bindings are difficult to follow with all sort
of opaque requisite things todo. I'd prefer to leave the name more explicit so
somebody tripping thru here in the future doesn't think there is a generic
toV8() somewhere that magically does the right thing.

http://codereview.chromium.org/113554/diff/251/266
File html/HTMLHtmlElement.cpp (right):

http://codereview.chromium.org/113554/diff/251/266#newcode33
Line 33: #if ENABLE(OFFLINE_WEB_APPLICATIONS)
On 2009/05/27 16:30:35, Dimitri Glazkov wrote:
> I think you should put a FIXME here stating why we have to do this temporarily
> and your eventual intent to get rid of these.

Done.

http://codereview.chromium.org/113554/diff/251/273
File loader/FrameLoader.cpp (right):

http://codereview.chromium.org/113554/diff/251/273#newcode5327
Line 5327: 
On 2009/05/27 16:30:35, Dimitri Glazkov wrote:
> This can be just one line :) and brace on new line.

Done (doh?)

http://codereview.chromium.org/113554/diff/251/270
File loader/appcache2/ApplicationCacheFrontend.h (right):

http://codereview.chromium.org/113554/diff/251/270#newcode94
Line 94: class Delegate {
Right, Delegate --> Client

http://codereview.chromium.org/113554/diff/251/271
File loader/appcache2/DOMApplicationCache.h (right):

http://codereview.chromium.org/113554/diff/251/271#newcode115
Line 115: #if USE(V8)
The gist of the slimmer-trimmer v8 bindings technique involves setting these
event listeners by name at runtime. JSC uses the explicitly named methods above,
setOnfoo() and onfoo().

Somewhere in the code, we have to lookup by name, I put that here rather than in
the V8 bindings class. DOMWindow has similar methods for this purpose as well,
although it compiles them in always (not just for V8).

http://codereview.chromium.org/113554/diff/251/271#newcode151
Line 151: // Returns a pointer to the appropiate data member above
> Ditto. A RefPtr<Anything>* just looks plain weird.

Agreed :)

Although you understand, I'm mapping a name to one of the data members
immediately above in support of getting/setting/clearing by name (for v8) in
addition to the explicit setOnfoo/onfoo methods (for JSC).

Suggestions welcome.

I had considered putting the RefPtrs<> in a collection keyed by name, and
accessing the collection from both the by name and the explicit methods... but
the collection overhead didn't seem worth it and would involve more edits to the
existing code.

http://codereview.chromium.org/113554/diff/251/272
File loader/appcache2/DOMApplicationCache.idl (left):

http://codereview.chromium.org/113554/diff/251/272#oldcode46
Line 46: #if defined(ENABLE_APPLICATION_CACHE_DYNAMIC_ENTRIES) &&
ENABLE_APPLICATION_CACHE_DYNAMIC_ENTRIES
On 2009/05/27 16:30:35, Dimitri Glazkov wrote:
> Since you dropped ENABLE_APPLICATION_CACHE_DYNAMIC_ENTRIES here, perhaps that
> whole guard is not needed anywhere?

It is still an unimplemented feature of the old system albeit with some stubs in
place... i'm trying to not touch the old system in general so i'm not touching
that stuff.

http://codereview.chromium.org/113554/diff/251/260
File platform/network/ResourceResponseBase.h (right):

http://codereview.chromium.org/113554/diff/251/260#newcode172
Line 172: #endif
On 2009/05/27 16:30:35, Dimitri Glazkov wrote:
> It seems weird that we have both the ID and the manifest URL here. Why do we
> need that?

Good question... one that actually goes to how the feature is implemented!

The manifestURL is here to support immediate 'foreign' entry detection at
selectCacheWithManifest() time (in the frontend) without consulting the backend.
For that purpose, we really only need to know this value for 'main' resource
loads (perhaps this should only be guaranteed to be valid for 'main' resource
responses).

see FrameLoader::selectApplicationCacheWithManifest &
ApplicationCacheFrontend::selectFinalCacheWithManifest


> 
> Also, this may be a contentious issue -- adding to the size of the
> ResourceResponseBase.
> 
> If you do need both of them, perhaps these should be something like an
> SuperDuperAughmentedAppCacheID class, which includes manifest URL and the ID?

That wouldn't really do anything to decrease the size of things, but it would
encapsulate the appcache specific bits better. I'm gonna wait for other feedback
here.

[michaeln, 2009-05-27 23:12:55]

http://codereview.chromium.org/113554/diff/251/270
File loader/appcache2/ApplicationCacheFrontend.h (right):

http://codereview.chromium.org/113554/diff/251/270#newcode94
Line 94: class Delegate {
On 2009/05/27 21:55:01, michaeln wrote:
> Right, Delegate --> Client

Per discussions on the webkit list, got rid of the virtual methods altogether
and made ApplicationCacheFrontend have explicit knowlege of DOMApplicationCache,
so no more Delegate/Client interface... instead...

void setDOMApplicationCache(DOMApplicationCache* client);

[michaeln, 2009-05-27 23:48:50]

New snapshot uploaded

[dglazkov, 2009-05-29 17:04:36]

I think you're ready! :)

[michaeln, 2009-05-29 19:07:29]

On 2009/05/29 17:04:36, Dimitri Glazkov wrote:
> I think you're ready! :)

Yes, I put this in the webkit review Q a few days ago. If anyone has comments,
please make them there.
https://bugs.webkit.org/show_bug.cgi?id=25436

[andreip, 2009-06-08 16:11:16]

http://codereview.chromium.org/113554/diff/340/1295
File loader/appcache2/DOMApplicationCache.cpp (right):

http://codereview.chromium.org/113554/diff/340/1295#newcode219
Line 219: RefPtr<EventListener>*
DOMApplicationCache::getAttributeEventListenerStorage(const AtomicString&
eventType)
I replicated this change on Android and my reviewers made the following
comments:

//-----------------------
reed@google.com:

Maybe this is std, but returning a RefPtr to an embedded member seems risky,
since it implies (I think) that the caller can keep the reference longer than
you, but in this case that can't be, since in your destructor these will
implicitly go away.

//-----------------------
feng@google.com:

Do we need RefPtr<EventListener>* here? I think EventListener* should do that
purpose.
Returning the address at the middle of an object seems dangerous and the caller
does not change the ref count.

//-----------------------

I replied that

1. We should stay in sync with chromium / webkit so any changes should be made
here first.

2. This is just a private internal utility method. It needs to return a
RefPtr<>* since two of the call sites need to modify the RefPtr.

The counter argument to 2. is that this method may be made public later by
somebody else, which may lead to problems on the long run. This has happened
before.

[dglazkov, 2009-06-16 16:41:41]

I am starting to really worry about the size of this patch. Is there any way
this could be broken up into smaller pieces?

[jorlow, 2009-06-16 22:26:00]

On 2009/06/16 16:41:41, Dimitri Glazkov wrote:
> I am starting to really worry about the size of this patch. Is there any way
> this could be broken up into smaller pieces?

Agreed.  It's over 100KB now.  I stopped reviewing after getting through about
half the patch...was just too much to digest.

An obvious first split would be adding the defines in their own patch.  (Half of
these files are just that.)

http://codereview.chromium.org/113554/diff/5071/5084
File bindings/v8/custom/V8DOMApplicationCacheCustom.cpp (right):

http://codereview.chromium.org/113554/diff/5071/5084#newcode51
Line 51: #if 0 && ENABLE(WORKERS)
don't commit as is...if anything put a #error in if you're compiling appcache
and workers

http://codereview.chromium.org/113554/diff/5071/5086
File html/HTMLHtmlElement.cpp (right):

http://codereview.chromium.org/113554/diff/5071/5086#newcode63
Line 63: frame->loader()->selectApplicationCacheWithManifest(manifestURL);
can these ever be NULL?  (no idea...just asking)

http://codereview.chromium.org/113554/diff/5071/5094
File loader/FrameLoader.h (right):

http://codereview.chromium.org/113554/diff/5071/5094#newcode652
Line 652: OwnPtr<ApplicationCacheFrontend> m_appcacheFrontend;
so there's 1 per FrameLoader?

http://codereview.chromium.org/113554/diff/5071/5102
File loader/appcache2/ApplicationCacheBridge.h (right):

http://codereview.chromium.org/113554/diff/5071/5102#newcode62
Line 62: virtual void initializeContextAsync(const
GlobalApplicationCacheContextID &contextID,
Not sure if this matches up to webkit style.

http://codereview.chromium.org/113554/diff/5071/5087
File loader/appcache2/ApplicationCacheCommon.h (right):

http://codereview.chromium.org/113554/diff/5071/5087#newcode34
Line 34: #if ENABLE(APPLICATION_CACHE)
You probably should be consistent: either use guards in your files or in the
include sites...not both.

http://codereview.chromium.org/113554/diff/5071/5087#newcode51
Line 51: GlobalApplicationCacheContextID(ApplicationCacheContextID contextID,
int processID)
You also need a GlobalApplicationCacheContextID constructor to handle the setter
in ResourceRequestBase.h

http://codereview.chromium.org/113554/diff/5071/5087#newcode70
Line 70: static GlobalApplicationCacheContextID none() { return
GlobalApplicationCacheContextID(); }
this seems unnecessary

it also seems inefficient

http://codereview.chromium.org/113554/diff/5071/5091
File loader/appcache2/DOMApplicationCache.h (right):

http://codereview.chromium.org/113554/diff/5071/5091#newcode56
Line 56: UNCACHED = APPCACHE_UNCACHED,
Why not change everything to using APPCACHE_blah rather than creating this
redundant enum?

http://codereview.chromium.org/113554/diff/5071/5098
File loader/appcache2/SimpleApplicationCacheBridge.cpp (right):

http://codereview.chromium.org/113554/diff/5071/5098#newcode56
Line 56: delete intance->m_client;
I wonder if you should be using OwnPtr and such so that you don't have to manage
deletes yourself?

If so, you can just set them to 0 here.  If not, you need to set them to 0 after
doing the delete...and maybe add an ASSERT(instance->m_client)

http://codereview.chromium.org/113554/diff/5071/5076
File page/DOMWindow.idl (right):

http://codereview.chromium.org/113554/diff/5071/5076#newcode155
Line 155: #if (defined(ENABLE_OFFLINE_WEB_APPLICATIONS) &&
ENABLE_OFFLINE_WEB_APPLICATIONS) || (defined(ENABLE_APPLICATION_CACHE) &&
ENABLE_APPLICATION_CACHE)
use ENABLE(foo)

[michaeln, 2009-06-16 22:58:05]

Thanx for the comments...

The first increment (ApplicationCacheFrontent) was a more manageable size. The
deltas between that and the new additions, the second increment
(ApplicationCacheBridge), aren't too much either. But taken together, yup, its a
bunch to take in at once.

I don't intend to submit this as a single patch. I'm still working out how to
break it up into a sequence of patches that results in a fully functioning
system at every step. But until I do figure that out, I'm continuing to work on
the system incrementally from front to back (without having a fully functioning
system at each increment).

I'm working this way in part to figure out what that submittable sequence of
patches that don't break anything at each steop should be. Having code that
compiles and runs really helps inform me about that :)

http://codereview.chromium.org/113554/diff/5071/5084
File bindings/v8/custom/V8DOMApplicationCacheCustom.cpp (right):

http://codereview.chromium.org/113554/diff/5071/5084#newcode51
Line 51: #if 0 && ENABLE(WORKERS)
i'll replace this with a fixme

http://codereview.chromium.org/113554/diff/5071/5086
File html/HTMLHtmlElement.cpp (right):

http://codereview.chromium.org/113554/diff/5071/5086#newcode63
Line 63: frame->loader()->selectApplicationCacheWithManifest(manifestURL);
Nope, they can't

* the test on line 103 guarantees that there is a frame
* frame is gauranteed to have a loader

http://codereview.chromium.org/113554/diff/5071/5102
File loader/appcache2/ApplicationCacheBridge.h (right):

http://codereview.chromium.org/113554/diff/5071/5102#newcode62
Line 62: virtual void initializeContextAsync(const
GlobalApplicationCacheContextID &contextID,
> Not sure if this matches up to webkit style.

what specifically?

http://codereview.chromium.org/113554/diff/5071/5087
File loader/appcache2/ApplicationCacheCommon.h (right):

http://codereview.chromium.org/113554/diff/5071/5087#newcode34
Line 34: #if ENABLE(APPLICATION_CACHE)
i think i'm following precedent for how other optional subsystems are handled...
which is guard at both the include sites and in the include file

i kind of like it that way, easier to tell if its relevant when looking at
either location

http://codereview.chromium.org/113554/diff/5071/5087#newcode51
Line 51: GlobalApplicationCacheContextID(ApplicationCacheContextID contextID,
int processID)
that uses the default copy ctor which copies data members.. just ints... a
simple struct assignment

http://codereview.chromium.org/113554/diff/5071/5087#newcode70
Line 70: static GlobalApplicationCacheContextID none() { return
GlobalApplicationCacheContextID(); }
ok, i make this a const ID& return value

http://codereview.chromium.org/113554/diff/5071/5098
File loader/appcache2/SimpleApplicationCacheBridge.cpp (right):

http://codereview.chromium.org/113554/diff/5071/5098#newcode56
Line 56: delete intance->m_client;
The bridge doesn't own the client. The static startup() method creates above
creates it, so its balanced here in the shutdown(). call.

There are other uses of this class where a different impl of that interface will
be given to the SimpleApplicationCacheBridge object.

This is a fairly standard pattern in webkit where the 'foo' doesn't own the
'fooClient'.

http://codereview.chromium.org/113554/diff/5071/5076
File page/DOMWindow.idl (right):

http://codereview.chromium.org/113554/diff/5071/5076#newcode155
Line 155: #if (defined(ENABLE_OFFLINE_WEB_APPLICATIONS) &&
ENABLE_OFFLINE_WEB_APPLICATIONS) || (defined(ENABLE_APPLICATION_CACHE) &&
ENABLE_APPLICATION_CACHE)
I don't think the IDL compliler deals with that. See other usages of ENABLE
defines in this file.

[dglazkov, 2009-06-17 16:20:18]

I understand how this will work. And I believe you're on track there. I do like
the birth of *Bridge. I have a couple of concerns:

* Adding a KURL to a ResourceResponseBase seems wrong. That boy is heavy and
will definitely have an impact on memory use. Perhaps we could come up with some
other ways to make this lighter?

* I am struggling with the naming. Anytime you have an *Impl, you know you're in
the woods :) They're ok, but I have no doubt there will be a discussion about
them upstream. I know you tried to solicit opinions on webkit-dev with no avail,
but just sayin' :)

Other than that, here are a few style nits:

http://codereview.chromium.org/113554/diff/5071/5102
File loader/appcache2/ApplicationCacheBridge.h (right):

http://codereview.chromium.org/113554/diff/5071/5102#newcode62
Line 62: virtual void initializeContextAsync(const
GlobalApplicationCacheContextID &contextID,
& with the type, no need to specify name unless it's needed for clarity. So just
(const GlobalApp...ID&);

http://codereview.chromium.org/113554/diff/5071/5102#newcode62
Line 62: virtual void initializeContextAsync(const
GlobalApplicationCacheContextID &contextID,
Since all of these are async, is Async suffix necessary?

http://codereview.chromium.org/113554/diff/5071/5102#newcode65
Line 65: virtual void uninitializeContextAsync(const
GlobalApplicationCacheContextID &contextID) = 0;
Ditto.

http://codereview.chromium.org/113554/diff/5071/5102#newcode67
Line 67: virtual void selectInitialCacheAsync(const
GlobalApplicationCacheContextID &contextID,
Ditto.

http://codereview.chromium.org/113554/diff/5071/5102#newcode103
Line 103: // This interface is used to by the bridge to call the frontend.
Client decl. usually goes on top.

http://codereview.chromium.org/113554/diff/5071/5103
File loader/appcache2/ApplicationCacheBridgeClientImpl.h (right):

http://codereview.chromium.org/113554/diff/5071/5103#newcode38
Line 38: 
<wtf/Vector.h>

http://codereview.chromium.org/113554/diff/5071/5087
File loader/appcache2/ApplicationCacheCommon.h (right):

http://codereview.chromium.org/113554/diff/5071/5087#newcode1
Line 1: /*
Use of *Common is not typical in WebKit. I'd suggest
GlobalApplicationCacheContextID.h and spread the constants close to decls that
use them.

http://codereview.chromium.org/113554/diff/5071/5098
File loader/appcache2/SimpleApplicationCacheBridge.cpp (right):

http://codereview.chromium.org/113554/diff/5071/5098#newcode42
Line 42: // static
BTW, no need to put the // static here and elsewhere. I did it once, and got
r-'d to remove them :)

http://codereview.chromium.org/113554/diff/5071/5098#newcode54
Line 54: SimpleApplicationCacheBridge* intance =
static_cast<SimpleApplicationCacheBridge*>(m_instance);
instance

[michaeln, 2009-06-23 19:21:31]

updated the snapshot... most notable change was class renaming

SimpleApplicationCacheBridge --> ApplicationCacheBridgeImpl

http://codereview.chromium.org/113554/diff/5071/5102
File loader/appcache2/ApplicationCacheBridge.h (right):

http://codereview.chromium.org/113554/diff/5071/5102#newcode62
Line 62: virtual void initializeContextAsync(const
GlobalApplicationCacheContextID &contextID,
On 2009/06/17 16:20:18, Dimitri Glazkov wrote:
> & with the type, no need to specify name unless it's needed for clarity. So
just
> (const GlobalApp...ID&);

I see, yes you're right.

I'm leaving this for now as I think an incremental step i'll have to take will
be to not have "global" ids at all. So I'll be revisiting this...

http://codereview.chromium.org/113554/diff/5071/5102#newcode103
Line 103: // This interface is used to by the bridge to call the frontend.
On 2009/06/17 16:20:18, Dimitri Glazkov wrote:
> Client decl. usually goes on top.

Done.

http://codereview.chromium.org/113554/diff/5071/5103
File loader/appcache2/ApplicationCacheBridgeClientImpl.h (right):

http://codereview.chromium.org/113554/diff/5071/5103#newcode38
Line 38: 
On 2009/06/17 16:20:18, Dimitri Glazkov wrote:
> <wtf/Vector.h>

Done.

http://codereview.chromium.org/113554/diff/5071/5087
File loader/appcache2/ApplicationCacheCommon.h (right):

http://codereview.chromium.org/113554/diff/5071/5087#newcode70
Line 70: static GlobalApplicationCacheContextID none() { return
GlobalApplicationCacheContextID(); }
i'm deferring doing anything here as i think i'll backing out "global" ids for
initial checkins... so since i'll be revisting this, not touching it now

http://codereview.chromium.org/113554/diff/5071/5098
File loader/appcache2/SimpleApplicationCacheBridge.cpp (right):

http://codereview.chromium.org/113554/diff/5071/5098#newcode42
Line 42: // static
this is a practice that i like (otherwise how can you tell by looking at the
method definition)...

i'll search and destroy these throughout later

http://codereview.chromium.org/113554/diff/5071/5098#newcode54
Line 54: SimpleApplicationCacheBridge* intance =
static_cast<SimpleApplicationCacheBridge*>(m_instance);
On 2009/06/17 16:20:18, Dimitri Glazkov wrote:
> instance

Done.