Bug 62302 – Remove "const" from obviously non-const accessors

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Peter Kasting]

Some accessors have obvious side effects, e.g. calling updateLayoutIgnorePendingStylesheets().  These should never be "const".  Other accessors call these and thus are transitively non-const as well.

There are also accessors that e.g. return collections of raw pointers that can be used to mutate state.  Where it isn't clear how to simply and efficiently provide a const version of these, we should probably mark them non-const as well.

[Peter Kasting]

Created attachment 97654 [details]
First patch v1

I also covered this in the ChangeLog, but for ease of reading/sorting, below are the functions made non-const, grouped as to why.  Sorry there's so many of these, but like most const-related patches, you wind up fixing chains of functions from a few source points :).  Please do look through these to see if I have any bogus rationale.

One unresolved question: Is it possible to provide a const version of Document::images() (and similar nearby accessors) that returns a collection of const pointers/objects that cannot be used to mutate the tree?  If so, we can avoid making some imageElement() accessors necessarily-non-const in this patch.


Functions marked newly non-const because they call existing functions with side effects:

Element::boundsInWindowSpace(): Calls Document::updateLayoutIgnorePendingStylesheets()
Element::cloneElementWithoutAttributesAndChildren(): Calls Document::createElement()
Element::getBoundingClientRect(): Calls Document::updateLayoutIgnorePendingStylesheets()
Element::getClientRects(): Calls Document::updateLayoutIgnorePendingStylesheets()
Element::innerText(): Calls Document::updateLayoutIgnorePendingStylesheets()
Element::scrollHeight(): Calls Document::updateLayoutIgnorePendingStylesheets()
Element::scrollLeft(): Calls Document::updateLayoutIgnorePendingStylesheets()
Element::scrollTop(): Calls Document::updateLayoutIgnorePendingStylesheets()
Element::scrollWidth(): Calls Document::updateLayoutIgnorePendingStylesheets()
HTMLAppletElement::renderWidgetForJSBindings(): Calls RenderApplet::createWidgetIfNecessary()
HTMLBodyElement::scrollHeight(): Calls Document::updateLayoutIgnorePendingStylesheets()
HTMLBodyElement::scrollLeft(): Calls Document::updateLayoutIgnorePendingStylesheets()
HTMLBodyElement::scrollTop(): Calls Document::updateLayoutIgnorePendingStylesheets()
HTMLBodyElement::scrollWidth(): Calls Document::updateLayoutIgnorePendingStylesheets()
HTMLEmbedElement::renderWidgetForJSBindings(): Calls Document::createElement()
HTMLFrameElementBase::height(): Calls Document::updateLayoutIgnorePendingStylesheets()
HTMLFrameElementBase::width(): Calls Document::updateLayoutIgnorePendingStylesheets()
HTMLImageElement::height():  Calls Document::updateLayoutIgnorePendingStylesheets()
HTMLImageElement::width(): Calls Document::updateLayoutIgnorePendingStylesheets()
HTMLMapElement::imageElement(): Calls Document::images()
HTMLObjectElement::renderWidgetForJSBindings(): Calls Document::updateLayoutIgnorePendingStylesheets()
HTMLOptionElement::selected(): Calls HTMLSelectElement::recalcListItemsIfNeeded()
Node::isContentEditable(): Calls Document::updateLayoutIgnorePendingStylesheets()
SVGElement::accessDocumentSVGExtensions():: Calls Document::accessSVGExtensions()
SVGSMILElement::targetElement(): Calls Document::accessSVGExtensions()
SVGStyledElement::buildPendingResourcesIfNeeded(): Calls Document::accessSVGExtensions()
SVGTextContentElement::getCharNumAtPosition(): Calls Document::updateLayoutIgnorePendingStylesheets()
SVGTextContentElement::getComputedTextLength(): Calls Document::updateLayoutIgnorePendingStylesheets()
SVGTextContentElement::getEndPositionOfChar(): Calls Document::updateLayoutIgnorePendingStylesheets()
SVGTextContentElement::getExtentOfChar(): Calls Document::updateLayoutIgnorePendingStylesheets()
SVGTextContentElement::getNumberOfChars(): Calls Document::updateLayoutIgnorePendingStylesheets()
SVGTextContentElement::getRotationOfChar(): Calls Document::updateLayoutIgnorePendingStylesheets()
SVGTextContentElement::getStartPositionOfChar(): Calls Document::updateLayoutIgnorePendingStylesheets()
SVGTextContentElement::getSubStringLength(): Calls Document::updateLayoutIgnorePendingStylesheets()
SVGUseElement::instanceRoot(): Calls Document::updateLayoutIgnorePendingStylesheets()
SVGUseElement::toClipPath(): Calls Document::accessSVGExtensions()


Static functions with a newly-non-const argument, for similar reasons:

static SVGLocatable::computeCTM(): Calls Document::updateLayoutIgnorePendingStylesheets() on arg
static SVGLocatable::getBBox(): Calls Document::updateLayoutIgnorePendingStylesheets() on arg


Virtual functions marked non-const because other implementations in the inheritance hierarchy are newly non-const:

HTMLInputElement::shouldUseInputMethod(): Node version is not const
HTMLPlugInElement::renderWidgetForJSBindings: Various subclass versions are not const
HTMLScriptElement::cloneElementWithoutAttributesAndChildren(): Element version is not const
HTMLTextAreaElement::shouldUseInputMethod(): Node version is not const
KeygenSelectElement::cloneElementWithoutAttributesAndChildren(): Element version is not const
OptionElement::selected(): HTMLOptionElement version is not const
SliderThumbElement::cloneElementWithoutAttributesAndChildren(): Element version is not const
SVGLocatable::getBBox(): Various subclass versions are not const
SVGLocatable::getCTM(): Various subclass versions are not const
SVGLocatable::getScreenCTM(): Various subclass versions are not const
SVGScriptElement::cloneElementWithoutAttributesAndChildren(): Element version is not const
SVGShadowTreeContainerElement::cloneElementWithoutAttributesOrChildren(): Element version is not const
SVGSMILElement::hasValidAttributeType(): Various subclass versions are not const
SVGStyledTransformableElement::toClipPath(): SVGUseElement version is not const


Functions marked newly non-const because they call other functions newly marked non-const:

Element::outerText(): Calls Element::innerText()
HTMLAnchorElement::text(): Calls Element::innerText()
HTMLAreaElement::imageElement(): Calls HTMLMapElement::imageElement()
HTMLPlugInElement::getInstance(): Calls HTMLPlugInElement::pluginWidget()
HTMLPlugInElement::pluginWidget(): Calls HTMLPlugInElement::renderWidgetForJSBindings()
Node::shouldUseInputMethod(): Calls Node::isContentEditable()
SVGAnimateElement::hasValidAttributeType(): Calls SVGSMILElement::targetElement()
SVGAnimateMotionElement::hasValidAttributeType(): Calls SVGSMILElement::targetElement()
SVGAnimateTransformElement::hasValidAttributeType(): Calls SVGSMILElement::targetElement()
SVGElement::boundingBox(): Calls SVGStyledLocatableElement::getBBox()
SVGLocatable::getTransformToElement(): Calls SVGLocatable::getCTM()
SVGSMILElement::eventBaseFor(): Calls SVGSMILElement::targetElement()
SVGStyledLocatableElement::getBBox(): Calls static SVGLocatable::getBBox()
SVGStyledLocatableElement::getCTM(): Calls static SVGLocatable::computeCTM()
SVGStyledLocatableElement::getScreenCTM(): Calls static SVGLocatable::computeCTM()
SVGStyledTransformableElement::getCTM(): Calls static SVGLocatable::computeCTM()
SVGStyledTransformableElement::getScreenCTM(): Calls static SVGLocatable::computeCTM()
SVGStyledTransformableElement::getBBox(): Calls static SVGLocatable::getBBox()
SVGTextContentElement::selectSubString(): Calls SVGTextContentElement::getNumberOfChars()
SVGTextElement::getBBox(): Calls static SVGLocatable::getBBox()
SVGTextElement::getCTM(): Calls static SVGLocatable::computeCTM()
SVGTextElement::getScreenCTM(): Calls static SVGLocatable::computeCTM()
WebElement::innerText(): Calls Element::innerText()


Finally, note the above reasons are not necessarily complete explanations, e.g. the cloneElementWithoutAttributesAndChildren() functions also generally create new objects, return pointers that allow tree mutation, etc.

[WebKit Review Bot]

Comment on attachment 97654 [details]
First patch v1

Attachment 97654 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8876801

[WebKit Review Bot]

Comment on attachment 97654 [details]
First patch v1

Attachment 97654 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8874764

[Darin Adler]

Comment on attachment 97654 [details]
First patch v1

Need to figure out why Mac EWS build failed.

[Peter Kasting]

Created attachment 97828 [details]
First patch v2

This should fix the Mac EWS build error.  I adjusted two function names in the exported symbols list to reflect their dropping of "const".

[Peter Kasting]

Any chance of a review on this?  I keep needing to fix small merge conflicts due to carrying this...

[Darin Adler]

Comment on attachment 97828 [details]
First patch v2

From reviewing the patch, I can’t tell if there is a mistake where we removed const from a function in a class and not from the same function in a derived class. That’s the main thing I’d worry about.

[WebKit Review Bot]

Comment on attachment 97828 [details]
First patch v2

Rejecting attachment 97828 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=ec2-cq-02', '--port..." exit_code: 2

Last 500 characters of output:
85 with fuzz 1 (offset -1 lines).
patching file Source/WebKit/chromium/src/WebSearchableFormData.cpp
patching file Source/WebKit/chromium/src/WebViewImpl.cpp
Hunk #1 succeeded at 1385 (offset 30 lines).
Hunk #2 succeeded at 1434 (offset 30 lines).
Hunk #3 FAILED at 1453.
1 out of 3 hunks FAILED -- saving rejects to file Source/WebKit/chromium/src/WebViewImpl.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Darin Adler', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/9099485

[Peter Kasting]

Created attachment 101501 [details]
First patch v3

Updated to apply to latest trunk.  I also manually rechecked all the virtual function changes to ensure that when one virtual function was changed, all others along the hierarchy were as well.  (This led me to add the missing "virtual" keyword to a couple of function declarations.)

[WebKit Review Bot]

Comment on attachment 101501 [details]
First patch v3

Clearing flags on attachment: 101501

Committed r91404: <http://trac.webkit.org/changeset/91404>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.