Bug 33186 - Track state for whether a ResourceRequest is fetched via SPDY
Summary: Track state for whether a ResourceRequest is fetched via SPDY
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Minor
Assignee: Mike Belshe
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-04 15:58 PST by Mike Belshe
Modified: 2010-05-13 13:13 PDT (History)
8 users (show)

See Also:


Attachments
patch (3.86 KB, patch)
2010-01-05 11:28 PST, Mike Belshe
no flags Details | Formatted Diff | Diff
patch fixing style nits (3.86 KB, patch)
2010-01-05 12:40 PST, Mike Belshe
no flags Details | Formatted Diff | Diff
Remove ifdef CHROMIUM flag as per David's suggestion (3.79 KB, patch)
2010-01-06 12:16 PST, Mike Belshe
fishd: review-
Details | Formatted Diff | Diff
Update with Darin's renaming suggestions. (3.85 KB, patch)
2010-01-06 15:31 PST, Mike Belshe
darin: review-
Details | Formatted Diff | Diff
Update with Darin A's comments. (4.28 KB, patch)
2010-01-06 16:31 PST, Mike Belshe
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Belshe 2010-01-04 15:58:24 PST
Track whether a ResourceRequest is fetched via the SPDY protocol.  Initially this will be chromium specific.
SPDY is part of an experiment to see if we can't build a faster web protocol:  http://dev.chromium.org/spdy.
Comment 1 Mike Belshe 2010-01-05 11:28:03 PST
Created attachment 45913 [details]
patch
Comment 2 WebKit Review Bot 2010-01-05 11:30:45 PST
Attachment 45913 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/src/WebURLResponse.cpp:258:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebKit/chromium/src/WebURLResponse.cpp:262:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2
Comment 3 Mike Belshe 2010-01-05 12:40:52 PST
Created attachment 45921 [details]
patch fixing style nits
Comment 4 WebKit Review Bot 2010-01-05 12:44:02 PST
style-queue ran check-webkit-style on attachment 45921 [details] without any errors.
Comment 5 Eric Seidel (no email) 2010-01-06 09:22:30 PST
I don't really understand the "isSPDY" naming scheme here.  Then again, I don't really know much about SPDY.  Does the protocol name change for SPDY?  like "spdy://"?  I'm just wondering if this is the right way to track this, and if "isSPDY" is the right name to do so.
Comment 6 Mike Belshe 2010-01-06 09:24:05 PST
Eric - the protocol scheme doesn't change, so there are no visible effects to see whether or not SPDY was used.  With this, I plan to enable some JS so that the page author can determine whether SPDY was used to deliver the page or not.
Comment 7 David Levin 2010-01-06 11:22:27 PST
Two comments:
1. It seems redundant that there is #if PLATFORM(CHROMIUM) inside of the chromium only file WebCore/platform/network/chromium/ResourceResponse.h

2. Note that this flag will not be copied for workers because that is done in WebCore/platform/network/ResourceResponseBase.h  I don't know if that is important to this change (or will be in the future). If it is, then I can tell you how to fix this.
Comment 8 Mike Belshe 2010-01-06 12:16:25 PST
Created attachment 45977 [details]
Remove ifdef CHROMIUM flag as per David's suggestion
Comment 9 Mike Belshe 2010-01-06 12:18:03 PST
Thanks David - I removed the ifdef.  This isn't needed in the Workers - I am only going to expose this for the main frame URLs anyway.  If I need changes in the future, we can add that.
Comment 10 WebKit Review Bot 2010-01-06 12:20:42 PST
style-queue ran check-webkit-style on attachment 45977 [details] without any errors.
Comment 11 Darin Fisher (:fishd, Google) 2010-01-06 13:23:59 PST
Comment on attachment 45977 [details]
Remove ifdef CHROMIUM flag as per David's suggestion

> Index: WebCore/platform/network/chromium/ResourceResponse.h
...
> +        // If this request was fetched via SPDY.
> +        bool m_isSpdy;

How about naming this m_fetchedViaSPDY?  Then, the comment can be
deleted.  You should similarly rename the getter and setter.

Same goes for the corresponding WebURLResponse methods.

With those changes, LGTM.
Comment 12 Darin Adler 2010-01-06 13:25:57 PST
Comment on attachment 45977 [details]
Remove ifdef CHROMIUM flag as per David's suggestion

For boolean data and function members we try to make them work as if they were in this sentence: "response <xxx>". So something like wasFetchedViaSPDY would be an example of a good name.
Comment 13 Mike Belshe 2010-01-06 15:31:32 PST
Created attachment 46001 [details]
Update with Darin's renaming suggestions.
Comment 14 WebKit Review Bot 2010-01-06 15:34:18 PST
style-queue ran check-webkit-style on attachment 46001 [details] without any errors.
Comment 15 Darin Adler 2010-01-06 15:59:46 PST
(In reply to comment #13)
> Update with Darin's renaming suggestions.

You mean Darins’ renaming suggestions ;-)
Comment 16 Mike Belshe 2010-01-06 16:03:54 PST
yes! :-)
Comment 17 Darin Adler 2010-01-06 16:08:00 PST
Comment on attachment 46001 [details]
Update with Darin's renaming suggestions.

If the name is SPDY, the we would prefer to have that in the identifiers, not Spdy.

> +        Enabled only in Chromium.

No longer true.

> +        * platform/network/chromium/ResourceResponse.h:
> +        (WebCore::ResourceResponse::isSpdy):
> +        (WebCore::ResourceResponse::setIsSpdy):

Names here out of date.

Where is m_fetchedViaSpdy initialized to false? Normally constructors should initialize all members.

> +        void setWasFetchedViaSpdy(const bool value)

The const here isn't needed or helpful.

> +        bool m_fetchedViaSpdy;

Even data boolean members work well with the "class-name <xxx>" naming scheme, so I would call this m_wasFetchedViaSpdy.

> +        Enabled only in Chromium.

Not true any more. And not really important to add in a Chromium directory ChangeLog.

> +        * src/WebURLResponse.cpp:
> +        (WebKit::WebURLResponse::isSpdy):
> +        (WebKit::WebURLResponse::setIsSpdy):

Names here out of date.

review- because of this round of nitpicks -- seems generally good
Comment 18 Mike Belshe 2010-01-06 16:31:14 PST
Created attachment 46007 [details]
Update with Darin A's comments.

Thanks Darin A. - comments addressed; sorry for my lameness.
Comment 19 WebKit Review Bot 2010-01-06 16:37:09 PST
style-queue ran check-webkit-style on attachment 46007 [details] without any errors.
Comment 20 WebKit Commit Bot 2010-01-06 16:54:58 PST
Comment on attachment 46007 [details]
Update with Darin A's comments.

Rejecting patch 46007 from commit-queue.

mbelshe@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/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/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 21 WebKit Commit Bot 2010-01-06 20:06:33 PST
Comment on attachment 46007 [details]
Update with Darin A's comments.

Clearing flags on attachment: 46007

Committed r52898: <http://trac.webkit.org/changeset/52898>
Comment 22 WebKit Commit Bot 2010-01-06 20:06:40 PST
All reviewed patches have been landed.  Closing bug.
Comment 23 Darin Fisher (:fishd, Google) 2010-01-06 22:24:50 PST
> If the name is SPDY, the we would prefer to have that in the identifiers, not
> Spdy.

It seems like this change wasn't applied in the final patch.  Oversight?
Comment 24 Mike Belshe 2010-01-06 22:45:53 PST
(In reply to comment #23)
> > If the name is SPDY, the we would prefer to have that in the identifiers, not
> > Spdy.
> 
> It seems like this change wasn't applied in the final patch.  Oversight?

Sorry- forgot to comment on that.  In the rest of chromium, we're going with Spdy, not SPDY, to match our use of Http, not HTTP.  So I was making this match.
Comment 25 Mark Rowe (bdash) 2010-01-07 01:51:45 PST
(In reply to comment #24)
> Sorry- forgot to comment on that.  In the rest of chromium, we're going with
> Spdy, not SPDY, to match our use of Http, not HTTP.  So I was making this
> match.

Surely code in WebCore should be following WebKit coding conventions.
Comment 26 Mark Rowe (bdash) 2010-01-07 01:52:53 PST
I also see no mention of a reviewer in <http://trac.webkit.org/changeset/52898>.  What’s up with that?
Comment 27 Mike Belshe 2010-01-07 09:21:20 PST
(In reply to comment #25)
> (In reply to comment #24)
> > Sorry- forgot to comment on that.  In the rest of chromium, we're going with
> > Spdy, not SPDY, to match our use of Http, not HTTP.  So I was making this
> > match.
> 
> Surely code in WebCore should be following WebKit coding conventions.

WebCore is inconsistent with this today; let me know if a "correct" casing is documented somewhere.  Hopefully this is not a significant issue.

Some examples from other code pre-existing in WebCore:
WebCore/platform/KURL.h:   procotolInHTTPFamily()
WebCore/dom/Document.h:    processHttpEquiv()
Qt port:                   isHttpOnly() and ignoreHttpError()
etc.
Comment 28 Mike Belshe 2010-01-07 09:23:46 PST
(In reply to comment #26)
> I also see no mention of a reviewer in
> <http://trac.webkit.org/changeset/52898>.  What’s up with that?

Sorry, my mistake.  So when people do code reviews, you first submit the review, then mark it review:?.  At this point, you don't know who the reviewer is going to be, so you leave it blank.  Later, after someone reviews it, you create a whole new patch changing the ChangeLog entry and upload that?  Sorry for the mistake.
Comment 29 Darin Adler 2010-01-07 09:25:30 PST
(In reply to comment #27)
> WebCore/platform/KURL.h:   procotolInHTTPFamily()

New code, represents our project style. This *is* the WebKit style.

> WebCore/dom/Document.h:    processHttpEquiv()

Very old code, dates back to the KHTML days.'

> Qt port:                   isHttpOnly() and ignoreHttpError()

Non-cross-port code often receives less-close scrutiny.

The WebKit style is to keep acronyms capitalized. This is explicit in <http://webkit.org/coding/coding-style.html>.
Comment 30 Darin Adler 2010-01-07 09:28:11 PST
(In reply to comment #28)
> Sorry, my mistake.  So when people do code reviews, you first submit the
> review, then mark it review:?.  At this point, you don't know who the reviewer
> is going to be, so you leave it blank.

You leave it in exactly this format.

    Reviewed by NOBODY (OOPS!).

This helps if you forget, because if you try to commit without changing that line the pre-commit hook will prevent you from checking in.

> Later, after someone reviews it, you
> create a whole new patch changing the ChangeLog entry and upload that?

No.

The committer is responsible for putting in the reviewer name. When the committer is a real live person there is some flexibility about what format the Reviewed by line is in. When the committer is the commit-queue script then it has to be exactly the correct format.
Comment 31 Mike Belshe 2010-01-07 09:36:56 PST
(In reply to comment #29)
> (In reply to comment #27)
> > WebCore/platform/KURL.h:   procotolInHTTPFamily()
> 
> New code, represents our project style. This *is* the WebKit style.
> 
> > WebCore/dom/Document.h:    processHttpEquiv()
> 
> Very old code, dates back to the KHTML days.'
> 
> > Qt port:                   isHttpOnly() and ignoreHttpError()
> 
> Non-cross-port code often receives less-close scrutiny.
> 
> The WebKit style is to keep acronyms capitalized. This is explicit in
> <http://webkit.org/coding/coding-style.html>.

I'll update.
Comment 32 Mike Belshe 2010-01-07 09:46:36 PST
Reopening to fix style issues.
Comment 33 Mike Belshe 2010-01-07 09:58:45 PST
On second thought, I better file a new bug.  I'm pretty sure someone will yell at me if I try to attach a style patch onto this bug.
Comment 34 Adam Barth 2010-05-13 13:13:07 PDT
(In reply to comment #33)
> On second thought, I better file a new bug.  I'm pretty sure someone will yell at me if I try to attach a style patch onto this bug.

Thanks!  :)