[webkit-reviews] review granted: [Bug 27821] ApplicationCacheHost refactoring : [Attachment 34026] Patch to address ap's comments

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Tue Aug 4 13:33:47 PDT 2009


Alexey Proskuryakov <ap at webkit.org> has granted Michael Nordman
<michaeln at google.com>'s request for review:
Bug 27821: ApplicationCacheHost refactoring
https://bugs.webkit.org/show_bug.cgi?id=27821

Attachment 34026: Patch to address ap's comments
https://bugs.webkit.org/attachment.cgi?id=34026&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
> +	   ApplicationCacheHost is held in an OwnPtr<> by DocumentLoader.

It would be good to briefly explain why this was done, not just mention what
was done. This would both provide useful information to folks skimming over
commit logs, and make it easier to discover reasons for changes after svn
blame. Of course, there is always a choice between having the explanations in
Bugzilla and having them in ChangeLogs and having tham in code comments - I
think that this case is definitely a candidate for ChangeLog.

> +	   Cleanup ussage of ENABLE(xxx) around includes.

Ditto. The rest of ChangeLog comments look fine to me - we don't usually add
blank lines, but it's not a big deal.

> -    class ResourceRequest;
> +    struct ResourceRequest;

There is a mismatch between platforms - in
platform/mac/network/ResourceRequest.h, it's a class. I don't know why it's a
struct on some other platforms, looks like a bug to me. This should be fixed,
but in a separate patch.

This is a great set of improvements, r=me. Please undo the struct/class change
though.


More information about the webkit-reviews mailing list