Bug 35317 – [Chromium] Implement WebLabelElement

• Component: WebKit API
• Status: RESOLVED FIXED

[James Hawkins]

Patch will follow.

[James Hawkins]

Created attachment 49334 [details]
[Chromium] Implement WebLabelElement

[WebKit Review Bot]

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.

[James Hawkins]

Re: style bot.

This addition matches the style of WebInputElement.h, should I ignore the errors or fix them?

[James Hawkins]

Created attachment 49346 [details]
[Chromium] Implement WebLabelElement [try2]

I decided to go ahead and fix the style errors.

[Darin Fisher (:fishd, Google)]

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.


> +#if WEBKIT_IMPLEMENTATION
> +    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'

[James Hawkins]

Created attachment 49444 [details]
[Chromium] Implement WebLabelElement [try3]

Fixed nits.

[Darin Fisher (:fishd, Google)]

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!

[James Hawkins]

Created attachment 49448 [details]
[Chromium] Implement WebDocument::getElementsByTagName [try4]

Fixes remaining s/n/e/ nits that I missed in the last patch.

[WebKit Commit Bot]

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

[James Hawkins]

Created attachment 49508 [details]
[Chromium] Implement WebLabelElement [try5]

I edited the Changelog file itself this time instead of the patch...doh.

[WebKit Commit Bot]

Comment on attachment 49508 [details]
[Chromium] Implement WebLabelElement [try5]

Clearing flags on attachment: 49508

Committed r55254: <http://trac.webkit.org/changeset/55254>

[Eric Seidel (no email)]

Not sure what the status is here.  Is [try5] supposed to be reviewed and landed?

[James Hawkins]

Yea, try5 was committed.  Wasn't marked as fixed for some reason?

[Eric Seidel (no email)]

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. :)

[Eric Seidel (no email)]

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.