Bug 51791 – Eliminate HTMLInputElement::m_deprecatedTypeNumber, other input refactoring and renaming

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Darin Adler]

Eliminate HTMLInputElement::m_deprecatedTypeNumber, other input refactoring and renaming

[Darin Adler]

Created attachment 77745 [details]
Patch

[Darin Adler]

Created attachment 77770 [details]
Patch

[WebKit Review Bot]

Attachment 77770 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7233356

[WebKit Review Bot]

Attachment 77770 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7319282

[Dimitri Glazkov (Google)]

Comment on attachment 77770 [details]
Patch

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

This is awesome, long collective work of factoring this out finally coming to a close. Almost wish we landed this patch in 2010 :)

> WebCore/html/HTMLInputElement.cpp:604
> +        if (renderer() && renderer()->isRenderImage())

Seems like this could be a flag on the InputType?

> WebCore/html/HTMLInputElement.cpp:-1394
> -bool HTMLInputElement::defaultChecked() const
> -{
> -    return fastHasAttribute(checkedAttr);
> -}
> -

The EWS says that it's used it in WebKit/chromium/src/WebSearchableFormData.cpp. Can you change the code there to just query the attribute?

> WebCore/html/InputType.h:75
> +    // Type query functions (try not to use these; bad factoring)

Can you explain a bit more here, maybe indicate future intent of refactoring?

[Darin Adler]

(In reply to comment #5)
> > WebCore/html/HTMLInputElement.cpp:604
> > +        if (renderer() && renderer()->isRenderImage())
> 
> Seems like this could be a flag on the InputType?

I made an altAttributeChanged function. With that in place, HTMLInputElement doesn't even need to include RenderImage.h.

> > WebCore/html/HTMLInputElement.cpp:-1394
> > -bool HTMLInputElement::defaultChecked() const
> > -{
> > -    return fastHasAttribute(checkedAttr);
> > -}
> > -
> 
> The EWS says that it's used it in WebKit/chromium/src/WebSearchableFormData.cpp. Can you change the code there to just query the attribute?

Done.

> > WebCore/html/InputType.h:75
> > +    // Type query functions (try not to use these; bad factoring)
> 
> Can you explain a bit more here, maybe indicate future intent of refactoring?

Will write a better comment.

[Darin Adler]

Created attachment 77789 [details]
Patch

[Kent Tamura]

Comment on attachment 77789 [details]
Patch

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

Great!

> WebCore/html/BaseTextInputType.h:43
> +private:

nit: Should have a blank line before private:

> WebCore/html/ImageInputType.cpp:100
> +     image->updateAltText();

nit: wrong indentation.

[Darin Adler]

(In reply to comment #8)
> > WebCore/html/BaseTextInputType.h:43
> > +private:
> 
> nit: Should have a blank line before private:

Thanks. Fixed.

> > WebCore/html/ImageInputType.cpp:100
> > +     image->updateAltText();
> 
> nit: wrong indentation.

Fixed.

[Darin Adler]

Committed r74895: <http://trac.webkit.org/changeset/74895>

[WebKit Review Bot]

http://trac.webkit.org/changeset/74895 might have broken Qt Linux Release minimal

[Daniel Bates]

Landed in changeset 74899 <http://trac.webkit.org/changeset/74899> the portion of the patch for file WebKit/chromium/src/WebSearchableFormData.cpp. For some reason only the change log corresponding to this change was landed.

[Daniel Bates]

For historical preservation, Csaba Osztrogonac landed the Qt build fix in changeset 74900 <http://trac.webkit.org/changeset/74900>.

From briefly looking at the Qt build files, I'm unclear why the Qt Linux Release Minimal bot is the only bot that complains about the missing ExceptionCode.h.

[Darin Adler]

(In reply to comment #13)
> For historical preservation, Csaba Osztrogonac landed the Qt build fix in changeset 74900 <http://trac.webkit.org/changeset/74900>.
> 
> From briefly looking at the Qt build files, I'm unclear why the Qt Linux Release Minimal bot is the only bot that complains about the missing ExceptionCode.h.

I presume that in other configurations some other header is including ExceptionCode.h.

[Darin Adler]

(In reply to comment #12)
> Landed in changeset 74899 <http://trac.webkit.org/changeset/74899> the portion of the patch for file WebKit/chromium/src/WebSearchableFormData.cpp. For some reason only the change log corresponding to this change was landed.

Strange. I landed using "webkit-patch land" and I believe the change was in my tree at that time. It was the same computer where I earlier used "webkit-patch upload".