Bug 84009 - Introduce an internal feature for a fixed placeholder
Summary: Introduce an internal feature for a fixed placeholder
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Kent Tamura
URL:
Keywords:
Depends on:
Blocks: 83872
  Show dependency treegraph
 
Reported: 2012-04-15 23:29 PDT by Kent Tamura
Modified: 2012-04-17 01:37 PDT (History)
3 users (show)

See Also:


Attachments
Patch (6.37 KB, patch)
2012-04-15 23:38 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 2 (6.46 KB, patch)
2012-04-16 01:38 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 3 (6.40 KB, patch)
2012-04-16 01:39 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff
Patch 4 (6.65 KB, patch)
2012-04-17 00:09 PDT, Kent Tamura
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kent Tamura 2012-04-15 23:29:49 PDT
Introduce an internal feature for a fixed placeholder
Comment 1 Kent Tamura 2012-04-15 23:38:52 PDT
Created attachment 137282 [details]
Patch
Comment 2 Kentaro Hara 2012-04-16 01:22:36 PDT
Comment on attachment 137282 [details]
Patch

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

> Source/WebCore/html/InputType.h:243
> +    // If supportsPlaceholder() && defaultPlaceholder().isEmpty(), it means a
> +    // type supports the 'placeholder' attribute.
> +    // If supportsPlaceholder() && !defaultPlaceholder().isEmpty(), it means a
> +    // type doesn't support the 'placeholder' attribute, but shows
> +    // defaultPlaceholder() string as a placeholder.

Nit: It might be better to put this comment above supportsPlaceholder().

> Source/WebCore/html/TextFieldInputType.cpp:397
> +    if (placeholderText.isEmpty())

Shouldn't this be 'if (!placeholderText.isEmpty())'?
Comment 3 Kent Tamura 2012-04-16 01:37:15 PDT
Comment on attachment 137282 [details]
Patch

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

>> Source/WebCore/html/InputType.h:243
>> +    // defaultPlaceholder() string as a placeholder.
> 
> Nit: It might be better to put this comment above supportsPlaceholder().

It's reasonable.  I'll do.

>> Source/WebCore/html/TextFieldInputType.cpp:397
>> +    if (placeholderText.isEmpty())
> 
> Shouldn't this be 'if (!placeholderText.isEmpty())'?

No.  If defaultPlaceholder() is empty, we'll use the 'placeholder' attribute value.
Comment 4 Kent Tamura 2012-04-16 01:38:27 PDT
Created attachment 137302 [details]
Patch 2

Fix a comment
Comment 5 Kent Tamura 2012-04-16 01:39:55 PDT
Created attachment 137303 [details]
Patch 3
Comment 6 Kentaro Hara 2012-04-16 01:40:41 PDT
Comment on attachment 137282 [details]
Patch

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

>>> Source/WebCore/html/TextFieldInputType.cpp:397
>>> +    if (placeholderText.isEmpty())
>> 
>> Shouldn't this be 'if (!placeholderText.isEmpty())'?
> 
> No.  If defaultPlaceholder() is empty, we'll use the 'placeholder' attribute value.

Ah, got it. I'd been confused.

> Source/WebCore/html/TextFieldInputType.cpp:398
> +        placeholderText = element()->strippedPlaceholder();

Nit: You can move this statement inside the below if statement.
Comment 7 Kentaro Hara 2012-04-16 01:41:41 PDT
(In reply to comment #6)
> > Source/WebCore/html/TextFieldInputType.cpp:398
> > +        placeholderText = element()->strippedPlaceholder();
> 
> Nit: You can move this statement inside the below if statement.

Sorry, ignore this comment. You code is right.
Comment 8 Kent Tamura 2012-04-17 00:09:47 PDT
Created attachment 137486 [details]
Patch 4

Add InputType::usesFixedPlaceholder()
Comment 9 WebKit Review Bot 2012-04-17 01:36:57 PDT
Comment on attachment 137486 [details]
Patch 4

Clearing flags on attachment: 137486

Committed r114360: <http://trac.webkit.org/changeset/114360>
Comment 10 WebKit Review Bot 2012-04-17 01:37:02 PDT
All reviewed patches have been landed.  Closing bug.