Bug 35317

Summary: [Chromium] Implement WebLabelElement
Product: WebKit Reporter: James Hawkins <jhawkins>
Component: WebKit APIAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: commit-queue, eric, fishd, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: All   
Description Flags
[Chromium] Implement WebLabelElement
[Chromium] Implement WebLabelElement [try2]
fishd: review-
[Chromium] Implement WebLabelElement [try3]
fishd: review+, fishd: commit-queue-
[Chromium] Implement WebDocument::getElementsByTagName [try4]
fishd: review+, commit-queue: commit-queue-
[Chromium] Implement WebLabelElement [try5] none

Description James Hawkins 2010-02-23 15:47:54 PST
Patch will follow.
Comment 1 James Hawkins 2010-02-23 15:49:39 PST
Created attachment 49334 [details]
[Chromium] Implement WebLabelElement
Comment 2 WebKit Review Bot 2010-02-23 15:54:16 PST
Attachment 49334 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebKit/chromium/public/WebLabelElement.h:44:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
WebKit/chromium/public/WebLabelElement.h:49:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 3 files

If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 James Hawkins 2010-02-23 15:56:50 PST
Re: style bot.

This addition matches the style of WebInputElement.h, should I ignore the errors or fix them?
Comment 4 James Hawkins 2010-02-23 17:09:52 PST
Created attachment 49346 [details]
[Chromium] Implement WebLabelElement [try2]

I decided to go ahead and fix the style errors.
Comment 5 Darin Fisher (:fishd, Google) 2010-02-23 20:50:44 PST
Comment on attachment 49346 [details]
[Chromium] Implement WebLabelElement [try2]

> Index: WebKit/chromium/public/WebLabelElement.h

> +    WebLabelElement(const WebLabelElement& n) : WebElement(n) { }
> +
> +    WebLabelElement& operator=(const WebLabelElement& n)
> +    {
> +        WebElement::assign(n);
> +        return *this;
> +    }
> +
> +    WEBKIT_API void assign(const WebLabelElement& n) { WebElement::assign(n); }

^^^ nit:  s/n/e/ perhaps?  where "e" stands for element, but "n" doesn't seem to
be related to the type name WebLabelElement.

> +    WebLabelElement(const WTF::PassRefPtr<WebCore::HTMLLabelElement>&);
> +    WebLabelElement& operator=(const WTF::PassRefPtr<WebCore::HTMLLabelElement>&);
> +    operator WTF::PassRefPtr<WebCore::HTMLLabelElement>() const;
> +#endif
> +
> +    WEBKIT_API WebElement correspondingControl();
> +};

^^^ nit: please put the WEBKIT_IMPLEMENTATION section at the end of the class to
help make the actual public API a bit more visible.

> Index: WebKit/chromium/src/WebLabelElement.cpp

> +WebLabelElement::WebLabelElement(const WTF::PassRefPtr<HTMLLabelElement>& elem)
> +    : WebElement(elem)
> +{
> +}
> +
> +WebLabelElement& WebLabelElement::operator=(const WTF::PassRefPtr<HTMLLabelElement>& elem)

nit: no need to mention WTF:: since there is a global 'using namespace WTF'
Comment 6 James Hawkins 2010-02-24 14:52:00 PST
Created attachment 49444 [details]
[Chromium] Implement WebLabelElement [try3]

Fixed nits.
Comment 7 Darin Fisher (:fishd, Google) 2010-02-24 15:43:35 PST
Comment on attachment 49444 [details]
[Chromium] Implement WebLabelElement [try3]

> Index: WebKit/chromium/public/WebLabelElement.h

> +    WebLabelElement(const WebLabelElement& n) : WebElement(n) { }
> +
> +    WebLabelElement& operator=(const WebLabelElement& n)
> +    {
> +        WebElement::assign(n);
> +        return *this;

"n" -> "e" :)

r=me, but that name should be fixed before committing.  so if you could
upload another patch for the commit-queue, that'd be great.  thanks!
Comment 8 James Hawkins 2010-02-24 15:52:39 PST
Created attachment 49448 [details]
[Chromium] Implement WebDocument::getElementsByTagName [try4]

Fixes remaining s/n/e/ nits that I missed in the last patch.
Comment 9 WebKit Commit Bot 2010-02-24 22:40:31 PST
Comment on attachment 49448 [details]
[Chromium] Implement WebDocument::getElementsByTagName [try4]

Rejecting patch 49448 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Darin Fisher', '--force']" exit_code: 2
patching file WebKit/chromium/ChangeLog
patch: **** unexpected end of file in patch
patching file WebKit/chromium/WebKit.gyp
patching file WebKit/chromium/public/WebLabelElement.h
patching file WebKit/chromium/src/WebLabelElement.cpp

Full output: http://webkit-commit-queue.appspot.com/results/309362
Comment 10 James Hawkins 2010-02-25 10:23:54 PST
Created attachment 49508 [details]
[Chromium] Implement WebLabelElement [try5]

I edited the Changelog file itself this time instead of the patch...doh.
Comment 11 WebKit Commit Bot 2010-02-25 13:56:58 PST
Comment on attachment 49508 [details]
[Chromium] Implement WebLabelElement [try5]

Clearing flags on attachment: 49508

Committed r55254: <http://trac.webkit.org/changeset/55254>
Comment 12 Eric Seidel (no email) 2010-03-05 14:41:27 PST
Not sure what the status is here.  Is [try5] supposed to be reviewed and landed?
Comment 13 James Hawkins 2010-03-05 14:42:22 PST
Yea, try5 was committed.  Wasn't marked as fixed for some reason?
Comment 14 Eric Seidel (no email) 2010-03-05 14:44:53 PST
The commit-queue won't close bugs where other patches are marked r+/r?.  I finally caved and added this "feature"  to allow multi-patch bugs and after great protest from other users. :)
Comment 15 Eric Seidel (no email) 2010-03-05 14:45:31 PST
If folks use webkit-patch to upload their patches it will obsolete the previous patches.  In general that's a good idea so as not to confuse reviewers by having multiple patches up for review or marked r+, etc.