Bug 79313

Summary: IsSynchronizingStyleAttributeFlag could be purged
Product: WebKit Reporter: Hajime Morrita <morrita>
Component: DOMAssignee: Hajime Morrita <morrita>
Status: RESOLVED FIXED    
Severity: Normal CC: gustavo, kling, koivisto, pnormand, webkit.review.bot, xan.lopez
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 77860    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
morrita: review-
Patch v2 none

Hajime Morrita
Reported 2012-02-22 17:58:44 PST
It looks this flag can be implemented in another way to save a bit.
Attachments
Patch (4.81 KB, patch)
2012-02-22 21:48 PST, Hajime Morrita 		no flags
DetailsFormatted DiffDiff
Patch (8.49 KB, patch)
2012-02-23 16:55 PST, Hajime Morrita 		no flags
DetailsFormatted DiffDiff
Patch (8.84 KB, patch)
2012-02-23 17:44 PST, Hajime Morrita 		no flags
DetailsFormatted DiffDiff
Patch (9.38 KB, patch)
2012-02-27 16:39 PST, Andreas Kling 		morrita: review-
DetailsFormatted DiffDiff
Patch v2 (10.38 KB, patch)
2012-02-29 14:31 PST, Andreas Kling 		no flags
DetailsFormatted DiffDiff
Show Obsolete (4) View All Add attachment proposed patch, testcase, etc.
Hajime Morrita
Comment 1 2012-02-22 21:48:22 PST
Created attachment 128389 [details] Patch
Hajime Morrita
Comment 2 2012-02-22 21:49:43 PST
Kling, Antti, What do you think about this?
Antti Koivisto
Comment 3 2012-02-23 15:48:56 PST
Comment on attachment 128389 [details] Patch I don't see why this static needs be in Node. It should be pushed down to StyledElement. Even better would be figuring out how to get rid of this mechanism completely.
Hajime Morrita
Comment 4 2012-02-23 16:55:58 PST
Created attachment 128608 [details] Patch
Hajime Morrita
Comment 5 2012-02-23 16:57:19 PST
(In reply to comment #3) > I don't see why this static needs be in Node. It should be pushed down to StyledElement. Even better would be figuring out how to get rid of this mechanism completely. Right. I pushed down the flag to StyledElement. At this time, I yet found no good way to eliminate this flag entirely. Could you take another look?
Philippe Normand
Comment 6 2012-02-23 17:11:41 PST
Comment on attachment 128608 [details] Patch Attachment 128608 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/11600516
Hajime Morrita
Comment 7 2012-02-23 17:44:05 PST
Created attachment 128620 [details] Patch
Andreas Kling
Comment 8 2012-02-25 20:45:10 PST
Seems like it would be even cleaner to remove the flag and devise a way to update the style attribute without causing attributeChanged() & friends to be called.
Hajime Morrita
Comment 9 2012-02-26 17:14:50 PST
Comment on attachment 128620 [details] Patch Hmm. Okay, I'll investigate later. Clearing r? for now.
Andreas Kling
Comment 10 2012-02-27 16:39:36 PST
Created attachment 129127 [details] Patch Alternate approach.
WebKit Review Bot
Comment 11 2012-02-27 16:42:35 PST
Attachment 129127 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/dom/ElementAttributeData.h:72: The parameter name "name" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 1 in 7 files If any of these errors are false positives, please file a bug against check-webkit-style.
Hajime Morrita
Comment 12 2012-02-27 19:42:46 PST
Comment on attachment 129127 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=129127&action=review > Source/WebCore/dom/ElementAttributeData.cpp:205 > + It's sad to have this kind of duplication. How about to move Element::setAttributeInternal() here to ElementAttributeData::setAttribute() and pass an element for invoking callbacks? So that we can pass it a null to suppress callbacks and we'd no longer need this code duplication.
Hajime Morrita
Comment 13 2012-02-28 17:46:44 PST
Comment on attachment 129127 [details] Patch r- for pinging purpose. Feel free to turn r? again. It looks we need to fix style error anyway.
Andreas Kling
Comment 14 2012-02-29 14:31:48 PST
Created attachment 129519 [details] Patch v2 Add a 'notifyChanged' parameter to setAttribute() instead to minimize copypasta.
Andreas Kling
Comment 15 2012-02-29 17:03:40 PST
Comment on attachment 129519 [details] Patch v2 Clearing flags on attachment: 129519 Committed r109285: <http://trac.webkit.org/changeset/109285>
Andreas Kling
Comment 16 2012-02-29 17:03:50 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.