Bug 36698

Summary: There are build breaks when wml is enabled.
Product: WebKit Reporter: Gyuyoung Kim <gyuyoung.kim>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, eric, joone.hur, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Linux   
Attachments:
Description Flags
Patch for a build break due to the disabled() funciton
none
Patch for a build break due to the disabled() function
darin: review+, commit-queue: commit-queue-
Patch for a build break due to the disabled() function
eric: review-, eric: commit-queue-
Patch for a build break due to the disabled() function
eric: review-, commit-queue: commit-queue-
Patch for a build break due to the disabled() function none

Description Gyuyoung Kim 2010-03-27 02:55:58 PDT
Created attachment 51823 [details]
Patch for a build break due to the disabled() funciton

There is a build break due to the disabled() when wml is enabled. It seems that this break comes from the Bug 35056 - Disabled menu options are still submitted. 

WMLOptionElement class is a child class of OptionElement class as below. So, if a virtual function is added to OptionElement, WMLOptionElement class should defines that function as well.

  class WMLOptionElement : public WMLFormControlElement, public WMLEventHandlingElement, public OptionElement {



My modification is as below,

--- WebCore/wml/WMLOptionElement.h      (revision 56658)
+++ WebCore/wml/WMLOptionElement.h      (working copy)
@@ -41,6 +41,8 @@ public:
     virtual void childrenChanged(bool changedByParser = false, Node* beforeChange = 0, Node* afterChange = 0, int childCountDelta = 0);
     virtual void parseMappedAttribute(MappedAttribute*);

+    virtual bool disabled() const { return false; };
+

I think diabled() should be added to WMLOptionElement.h.

Thank you,
Gyuyoung Kim.
Comment 1 Gyuyoung Kim 2010-03-28 17:03:07 PDT
Created attachment 51872 [details]
Patch for a build break due to the disabled() function

I add a bug url to patch. Please review my patch.
Comment 2 Darin Adler 2010-03-29 09:28:25 PDT
Comment on attachment 51872 [details]
Patch for a build break due to the disabled() function

I'm not sure that's right. Is it true that WML option elements can't be disabled? We can follow up with another patch if the WML behavior is wrong, I guess. r=me
Comment 3 WebKit Commit Bot 2010-03-29 15:59:29 PDT
Comment on attachment 51872 [details]
Patch for a build break due to the disabled() function

Rejecting patch 51872 from commit-queue.

Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Darin Adler', '--force']" exit_code: 2
patching file WebCore/ChangeLog
patch: **** malformed patch at line 20:  

patching file WebCore/wml/WMLOptionElement.h

Full output: http://webkit-commit-queue.appspot.com/results/1626054
Comment 4 Gyuyoung Kim 2010-03-29 18:58:24 PDT
Created attachment 51993 [details]
 Patch for a build break due to the disabled() function 

When wml feature is enabled, there is a build break as below,

------------------------------------------------------------------------------
DerivedSources/WMLElementFactory.cpp: In function
‘WTF::PassRefPtr<WebCore::WMLElement> WebCore::optionConstructor(const
WebCore::QualifiedName&, WebCore::Document*, bool)’:
DerivedSources/WMLElementFactory.cpp:154: error: cannot allocate an object of
abstract type ‘WebCore::WMLOptionElement’
./WebCore/wml/WMLOptionElement.h:31: note:   because the following virtual
functions are pure within ‘WebCore::WMLOptionElement’:
./WebCore/dom/OptionElement.h:37: note:     virtual bool
WebCore::OptionElement::disabled() const
DerivedSources/WMLElementFactory.cpp: In function
‘WTF::PassRefPtr<WebCore::WMLElement> WebCore::selectConstructor(const
WebCore::QualifiedName&, WebCore::Document*, bool)’:
DerivedSources/WMLElementFactory.cpp:179: error: cannot allocate an object of
abstract type ‘WebCore::WMLSelectElement’
./WebCore/wml/WMLSelectElement.h:30: note:   because the following virtual
functions are pure within ‘WebCore::WMLSelectElement’:
./WebCore/dom/SelectElement.h:64: note:     virtual void
WebCore::SelectElement::listBoxSelectItem(int, bool, bool, bool)
------------------------------------------------------------------------------

I didn't check if the WML behavior is wrong. But, I think we need to avoid a build break for now.
I will report if WML option elements can't be disabled or not again. If I am wrong, please let me know. 

Thank you.
Comment 5 WebKit Review Bot 2010-03-29 19:01:30 PDT
Attachment 51993 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/wml/WMLOptionElement.h:44:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 Eric Seidel (no email) 2010-03-29 22:14:55 PDT
Comment on attachment 51993 [details]
 Patch for a build break due to the disabled() function 

Actually, no need for hte extra ";" after the }
Comment 7 Gyuyoung Kim 2010-03-29 22:35:18 PDT
Created attachment 52005 [details]
Patch for a build break due to the disabled() function   

Sorry for my frequent patches, I am not good at making a patch yet.

I found one more build break because of  Bug 36177 -  Multiselect Popup - Listbox click simulation.
(https://bugs.webkit.org/show_bug.cgi?id=36177)

The patch add a virtual function to SelectElement.h as below,
   virtual void listBoxSelectItem(int listIndex, bool allowMultiplySelections, bool shift, bool fireOnChangeNow = true) = 0;

It seems the patch of bug didn't consider wml. I add a virtual function to WMLSelectElement.h.
Comment 8 Eric Seidel (no email) 2010-03-29 22:40:51 PDT
Comment on attachment 52005 [details]
Patch for a build break due to the disabled() function   

OK.  Your ChangeLog does not mention WebCore/wml/WMLSelectElement.h, but it's OK.
Comment 9 Gyuyoung Kim 2010-03-29 22:42:46 PDT
Thanks. When I push another patches, I will write changelog correctly.
Comment 10 WebKit Commit Bot 2010-03-29 22:50:34 PDT
Comment on attachment 52005 [details]
Patch for a build break due to the disabled() function   

Rejecting patch 52005 from commit-queue.

Unexpected failure when landing patch!  Please file a bug against webkit-patch.
Failed to run "['WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', '--build-style=both', '--quiet', '52005', '--no-update']" exit_code: 1
Last 500 characters of output:
handle_errors
    self._run(tool, options, state)
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/tool/commands/stepsequence.py", line 60, in _run
    step(tool, options).run(state)
  File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 63, in run
    log("%s found in %s does not appear to be a valid reviewer according to committers.py." % (reviewer_text, changelog_path))
NameError: global name 'log' is not defined
Comment 11 Gyuyoung Kim 2010-03-29 23:00:56 PDT
My patch is wrong ?  Should I make a patch again ?
Comment 12 Eric Seidel (no email) 2010-03-29 23:01:49 PDT
Your patch looks fine.  Looks like Adam or I broke the commit-queue.  I'll investigate.
Comment 13 Eric Seidel (no email) 2010-03-29 23:14:26 PDT
Comment on attachment 52005 [details]
Patch for a build break due to the disabled() function   

Actually, looks like you modified the "reviewed by" line:
+        Reviewed by NOBODY(OOPS!).

the default template is:
+        Reviewed by NOBODY (OOPS!).

The scripts aren't smart enough to know the difference between "NOBODY(OOPS!)" and an intentionally specified reviewer.

I suggest just regenerating the ChangeLog from prepare-ChangeLog, which will both fix your reviewed by line to be the default as well as add the information about WebCore/wml/WMLSelectElement.h

Sorry our scripts are so troublesome.
Comment 14 Eric Seidel (no email) 2010-03-29 23:14:59 PDT
There was also a problem in webkit-patch's validatereviewer.py step which I've fixed.
Comment 15 Gyuyoung Kim 2010-03-29 23:31:10 PDT
Created attachment 52007 [details]
Patch for a build break due to the disabled() function

Thank you for your guide. I attach a patch again.
Comment 16 Eric Seidel (no email) 2010-03-29 23:45:45 PDT
Comment on attachment 52007 [details]
Patch for a build break due to the disabled() function

Thanks
Comment 17 WebKit Commit Bot 2010-03-30 00:08:10 PDT
Comment on attachment 52007 [details]
Patch for a build break due to the disabled() function

Clearing flags on attachment: 52007

Committed r56769: <http://trac.webkit.org/changeset/56769>
Comment 18 WebKit Commit Bot 2010-03-30 00:08:16 PDT
All reviewed patches have been landed.  Closing bug.