Bug 48185 - build-webkit: add support for --progress-tag switch
Summary: build-webkit: add support for --progress-tag switch
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords:
Depends on: 36961 48184 48210
Blocks:
  Show dependency treegraph
 
Reported: 2010-10-23 06:39 PDT by David Kilzer (:ddkilzer)
Modified: 2010-10-25 00:31 PDT (History)
6 users (show)

See Also:


Attachments
Patch v1 (1.68 KB, patch)
2010-10-23 06:43 PDT, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
proposed buildfix (909 bytes, patch)
2010-10-24 13:36 PDT, Csaba Osztrogonác
ddkilzer: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Kilzer (:ddkilzer) 2010-10-23 06:39:50 PDT
build-webkit: add support for --progress-tag switch
Comment 1 David Kilzer (:ddkilzer) 2010-10-23 06:43:34 PDT
Created attachment 71639 [details]
Patch v1
Comment 2 David Kilzer (:ddkilzer) 2010-10-24 07:00:10 PDT
Committed r70413: <http://trac.webkit.org/changeset/70413>
Comment 3 WebKit Review Bot 2010-10-24 10:06:07 PDT
http://trac.webkit.org/changeset/70413 might have broken Leopard Intel Debug (Tests)
The following tests are not passing:
editing/spelling/context-menu-suggestions.html
editing/spelling/spellcheck-attribute.html
editing/spelling/spelling-backspace-between-lines.html
editing/spelling/spelling-contenteditable.html
editing/spelling/spelling-textarea.html
platform/mac/accessibility/attributed-string-includes-misspelled-with-selection.html
platform/mac/accessibility/misspelled-attributed-string.html
Comment 4 Csaba Osztrogonác 2010-10-24 12:55:35 PDT
(In reply to comment #2)
> Committed r70413: <http://trac.webkit.org/changeset/70413>

It broke Qt minimal build with this error message:
(Unfortunately buildbot was dead.)

In file included from ../../../WebCore/html/HTMLMeterElement.cpp:32:
../../../WebCore/rendering/RenderMeter.h:35: error: expected class-name before ‘{’ token
../../../WebCore/rendering/RenderMeter.h: In function ‘WebCore::RenderMeter* WebCore::toRenderMeter(WebCore::RenderObject*)’:
../../../WebCore/rendering/RenderMeter.h:64: error: invalid static_cast from type ‘WebCore::RenderObject*’ to type ‘WebCore::RenderMeter*’
../../../WebCore/html/HTMLMeterElement.cpp: In member function ‘virtual WebCore::RenderObject* WebCore::HTMLMeterElement::createRenderer(WebCore::RenderArena*, WebCore::RenderStyle*)’:
../../../WebCore/html/HTMLMeterElement.cpp:54: error: cannot convert ‘WebCore::RenderMeter*’ to ‘WebCore::RenderObject*’ in return
g

It was rolled out by http://trac.webkit.org/changeset/70423 .
I'll check it later.
Comment 5 Csaba Osztrogonác 2010-10-24 12:56:10 PDT
Comment on attachment 71639 [details]
Patch v1

remove r+ from landed and rolled-out patch
Comment 6 Csaba Osztrogonác 2010-10-24 13:36:46 PDT
Created attachment 71696 [details]
proposed buildfix

Before this patch:
ENABLE_METER_TAG=1
ENABLE_PROGRESS_TAG=1

After this patch:
ENABLE_METER_TAG=1
ENABLE_PROGRESS_TAG=0

There were build fail, because METER_TAG depends on PROGRESS_TAG:
RenderMeter class inherited from RenderIndicator in rendering/RenderMeter.h
But after this patch RenderIndicator.h wasn't included in RenderMeter.h
It is only included undirectly by RenderProgress.h if PROGRESS_TAG is enabled.
I don't think if METER_TAG needs PROGRESS_TAG, but RenderIndicator class.
Comment 7 Csaba Osztrogonác 2010-10-24 13:37:24 PDT
(In reply to comment #6)
> Before this patch:
> After this patch:
I mean before/after the original patch.
Comment 8 Csaba Osztrogonác 2010-10-24 13:40:19 PDT
I think the original bug dependency set by sheriffbot was correct:
This bug depends on Bug 48210 -  REGRESSION(r70413): It broke Qt minimal build (Requested by Ossy on #webkit). and b48210 blocks this bug.
Comment 9 Csaba Osztrogonác 2010-10-24 13:44:03 PDT
Yael, you added HTMLMeterElement in http://trac.webkit.org/changeset/59541.
What do you think about my proposed fix ?

All, what do you think if we should add default value for ENABLE_METER_TAG,
same as for ENABLE_PROGRESS_TAG in http://trac.webkit.org/changeset/70413 ?
Comment 10 David Kilzer (:ddkilzer) 2010-10-24 13:49:56 PDT
Comment on attachment 71696 [details]
proposed buildfix

r=me.  This is obviously correct because RenderMeter derives from RenderIndicator.
Comment 11 David Kilzer (:ddkilzer) 2010-10-24 13:50:55 PDT
(In reply to comment #9)
> All, what do you think if we should add default value for ENABLE_METER_TAG,
> same as for ENABLE_PROGRESS_TAG in http://trac.webkit.org/changeset/70413 ?

That's fine.  I didn't even realize meter-tag was missing a switch on build-webkit, too.
Comment 12 Yael 2010-10-24 17:31:21 PDT
(In reply to comment #9)
> Yael, you added HTMLMeterElement in http://trac.webkit.org/changeset/59541.
> What do you think about my proposed fix ?
> 
Thanks for taking a quick action, Ossy :-)
You could also remove #include "RemderProgress.h". RenderMeter does not really depend on RenderProgress.

> All, what do you think if we should add default value for ENABLE_METER_TAG,
> same as for ENABLE_PROGRESS_TAG in http://trac.webkit.org/changeset/70413 ?
Is anyone doing this already? If not, I will do that tomorrow.
Comment 13 Csaba Osztrogonác 2010-10-25 00:26:51 PDT
(In reply to comment #10)
> (From update of attachment 71696 [details])
> r=me.  This is obviously correct because RenderMeter derives from RenderIndicator.

Landed in http://trac.webkit.org/changeset/70440.
Additionally I removed needless #include "RemderProgress.h".

Original patch relanded in http://trac.webkit.org/changeset/70441
Comment 14 Csaba Osztrogonác 2010-10-25 00:31:50 PDT
(In reply to comment #12)
> > All, what do you think if we should add default value for ENABLE_METER_TAG,
> > same as for ENABLE_PROGRESS_TAG in http://trac.webkit.org/changeset/70413 ?
> Is anyone doing this already? If not, I will do that tomorrow.

new bug filed on it: https://bugs.webkit.org/show_bug.cgi?id=48224