Bug 70276 - Remove OptionElement (first half)
Summary: Remove OptionElement (first half)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on: 70475
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-17 15:38 PDT by Darin Adler
Modified: 2011-10-20 18:26 PDT (History)
4 users (show)

See Also:


Attachments
Patch (42.05 KB, patch)
2011-10-17 15:48 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (42.08 KB, patch)
2011-10-17 16:46 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (42.01 KB, patch)
2011-10-18 15:34 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (43.77 KB, patch)
2011-10-18 17:47 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (45.54 KB, patch)
2011-10-19 12:22 PDT, Darin Adler
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2011-10-17 15:38:26 PDT
Remove OptionElement (first half)
Comment 1 Darin Adler 2011-10-17 15:48:27 PDT
Created attachment 111336 [details]
Patch
Comment 2 WebKit Review Bot 2011-10-17 15:49:59 PDT
Attachment 111336 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/update-webkit', '--chromium']" exit_code: 2

Updating OpenSource
Current branch master is up to date.
Updating chromium port dependencies using gclient...
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Re-trying 'depot_tools/gclient sync'
Error: Can't switch the checkout to http://v8.googlecode.com/svn/branches/3.6@9637; UUID don't match and there is local changes in /mnt/git/webkit-style-queue/Source/WebKit/chromium/v8. Delete the directory and try again.
Error: 'depot_tools/gclient sync' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 107.
Re-trying 'depot_tools/gclient sync'
No such file or directory at Tools/Scripts/update-webkit line 104.


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Early Warning System Bot 2011-10-17 16:17:25 PDT
Comment on attachment 111336 [details]
Patch

Attachment 111336 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10102320
Comment 4 Gyuyoung Kim 2011-10-17 16:32:11 PDT
Comment on attachment 111336 [details]
Patch

Attachment 111336 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10120022
Comment 5 Darin Adler 2011-10-17 16:46:30 PDT
Created attachment 111347 [details]
Patch
Comment 6 Early Warning System Bot 2011-10-17 17:21:04 PDT
Comment on attachment 111347 [details]
Patch

Attachment 111347 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10124027
Comment 7 Gyuyoung Kim 2011-10-17 17:50:27 PDT
Comment on attachment 111347 [details]
Patch

Attachment 111347 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10126035
Comment 8 WebKit Review Bot 2011-10-17 20:22:43 PDT
Comment on attachment 111347 [details]
Patch

Attachment 111347 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10120088
Comment 9 Kent Tamura 2011-10-18 04:04:06 PDT
Comment on attachment 111347 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111347&action=review

r- because of build failures.

> Source/WebCore/html/HTMLOptionElement.cpp:42
> +#include "StringBuilder.h"

Should be <wtf/text/StringBuilder.h>

> Source/WebCore/html/HTMLOptionElement.cpp:170
> +    int length = items.size();

nit: length should be size_t.

> Source/WebCore/html/HTMLOptionElement.cpp:315
> +    for (Node* n = firstChild(); n; ) {

nit: One-letter variable name is not good.

> Source/WebCore/html/HTMLSelectElement.cpp:78
> +static inline HTMLOptionElement* toOptionElement(Element* element)

This function name is confusing.  This looks like toClassName() which we use for many classes, but there is no OptionElement class and toClassName() usually returns no 0.
Do you have a plan to remove/rename this?
Comment 10 Darin Adler 2011-10-18 09:58:21 PDT
(In reply to comment #9)
> > Source/WebCore/html/HTMLOptionElement.cpp:315
> > +    for (Node* n = firstChild(); n; ) {
> 
> nit: One-letter variable name is not good.

I just moved that code, but I would be happy to give it a better name.

> > Source/WebCore/html/HTMLSelectElement.cpp:78
> > +static inline HTMLOptionElement* toOptionElement(Element* element)
> 
> This function name is confusing.  This looks like toClassName() which we use for many classes, but there is no OptionElement class and toClassName() usually returns no 0.
> Do you have a plan to remove/rename this?

Yes. I’ll do that in the second half.
Comment 11 Darin Adler 2011-10-18 10:09:53 PDT
Thanks for the review, by the way. I’ll try to upload a new cut at this soon.
Comment 12 Darin Adler 2011-10-18 15:34:25 PDT
Created attachment 111510 [details]
Patch
Comment 13 Kent Tamura 2011-10-18 16:01:34 PDT
Comment on attachment 111510 [details]
Patch

ok
Comment 14 WebKit Review Bot 2011-10-18 16:45:09 PDT
Comment on attachment 111510 [details]
Patch

Rejecting attachment 111510 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
omium/src/WebOptionElement.cpp: In member function 'void WebKit::WebOptionElement::setDefaultSelected(bool)':
Source/WebKit/chromium/src/WebOptionElement.cpp:71: error: 'class WebCore::HTMLOptionElement' has no member named 'setDefaultSelected'
Source/WebKit/chromium/src/WebOptionElement.cpp:71: error: return-statement with a value, in function returning 'void'
make: *** [out/Debug/obj.target/webkit/Source/WebKit/chromium/src/WebOptionElement.o] Error 1
make: *** Waiting for unfinished jobs....

Full output: http://queues.webkit.org/results/10138144
Comment 15 WebKit Review Bot 2011-10-18 17:00:19 PDT
Comment on attachment 111510 [details]
Patch

Attachment 111510 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10138163
Comment 16 Darin Adler 2011-10-18 17:47:09 PDT
Created attachment 111540 [details]
Patch
Comment 17 WebKit Review Bot 2011-10-18 18:43:15 PDT
Comment on attachment 111540 [details]
Patch

Attachment 111540 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10143189
Comment 18 Darin Adler 2011-10-19 12:22:48 PDT
Created attachment 111657 [details]
Patch
Comment 19 Kent Tamura 2011-10-19 15:47:21 PDT
Comment on attachment 111657 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111657&action=review

> Source/WebKit/chromium/src/WebOptionElement.cpp:72
> -    return unwrap<HTMLOptionElement>()->setDefaultSelected(newSelected);
> +    return unwrap<HTMLOptionElement>()->setAttribute(selectedAttr, newSelected ? "" : 0);

Probably we can use setBooleanAttribute().
Comment 20 Darin Adler 2011-10-19 15:49:25 PDT
setBooleanAttribute is a good idea. I had forgotten it existed!
Comment 21 WebKit Review Bot 2011-10-19 19:07:21 PDT
Comment on attachment 111657 [details]
Patch

Clearing flags on attachment: 111657

Committed r97917: <http://trac.webkit.org/changeset/97917>
Comment 22 WebKit Review Bot 2011-10-19 19:07:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 23 Kent Tamura 2011-10-20 02:59:23 PDT
Comment on attachment 111657 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111657&action=review

> Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:190
> -    RetainPtr<CGImageRef> image = copyNativeImage(copyBehavior);
> +    RetainPtr<CGImageRef> image(AdoptCF, copyNativeImage(copyBehavior));

Wow, this is unrelated to this bug.
Comment 24 Kent Tamura 2011-10-20 03:00:28 PDT
http://trac.webkit.org/changeset/97917 was rolled out.
Comment 25 Darin Adler 2011-10-20 18:00:39 PDT
Would have been much better to roll out that one unrelated line rather than rolling out the whole patch.
Comment 26 Darin Adler 2011-10-20 18:26:31 PDT
Committed r98053: <http://trac.webkit.org/changeset/98053>