Make calls to AtomicString(const String&) explicit in rendering/

Make calls to AtomicString(const String&) explicit in rendering/

Make calls to AtomicString(const String&) explicit in rendering/ and avoid
useless conversions from String to AtomicString when possible.

This is one step towards making the AtomicString(const String&) constructor
explicit. This CL takes care only of the rendering/ part so that it is more
easily reviewable.

R=eseidel
BUG=323739
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=164413

[Inactive, 2013-12-31 21:35:43]

https://codereview.chromium.org/122943003/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderThemeChromiumFontProvider.h (right):

https://codereview.chromium.org/122943003/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderThemeChromiumFontProvider.h:45: static const
WTF::AtomicString& defaultGUIFont();
All callers expect an AtomicString:
Source/core/rendering/RenderThemeChromiumFontProviderLinux.cpp:   
fontDescription.firstFamily().setFamily(defaultGUIFont());
Source/core/rendering/RenderThemeChromiumFontProviderWin.cpp:        faceName =
defaultGUIFont();

[eseidel, 2013-12-31 23:13:21]

I suspect that framgent() should return an AtomicString?  Otherwise lgtm.

On Tue, Dec 31, 2013 at 1:35 PM,  <ch.dumez@samsung.com> wrote:
> Reviewers: eseidel,
>
>
>
https://codereview.chromium.org/122943003/diff/1/Source/core/rendering/Render...
> File Source/core/rendering/RenderThemeChromiumFontProvider.h (right):
>
>
https://codereview.chromium.org/122943003/diff/1/Source/core/rendering/Render...
> Source/core/rendering/RenderThemeChromiumFontProvider.h:45: static const
> WTF::AtomicString& defaultGUIFont();
> All callers expect an AtomicString:
> Source/core/rendering/RenderThemeChromiumFontProviderLinux.cpp:
> fontDescription.firstFamily().setFamily(defaultGUIFont());
> Source/core/rendering/RenderThemeChromiumFontProviderWin.cpp:
> faceName = defaultGUIFont();
>
> Description:
> Make calls to AtomicString(const String&) explicit in rendering/
>
> Make calls to AtomicString(const String&) explicit in rendering/ and avoid
> useless conversions from String to AtomicString when possible.
>
> This is one step towards making the AtomicString(const String&) constructor
> explicit. This CL takes care only of the rendering/ part so that it is more
> easily reviewable.
>
> R=eseidel
> BUG=323739
>
> Please review this at https://codereview.chromium.org/122943003/
>
> SVN Base: https://chromium.googlesource.com/chromium/blink.git@master
>
> Affected files (+7, -7 lines):
>   M Source/core/rendering/FastTextAutosizer.cpp
>   M Source/core/rendering/HitTestResult.cpp
>   M Source/core/rendering/RenderLayer.cpp
>   M Source/core/rendering/RenderLayerFilterInfo.cpp
>   M Source/core/rendering/RenderThemeChromiumFontProvider.h
>   M Source/core/rendering/RenderThemeChromiumFontProvider.cpp
>
>
> Index: Source/core/rendering/FastTextAutosizer.cpp
> diff --git a/Source/core/rendering/FastTextAutosizer.cpp
> b/Source/core/rendering/FastTextAutosizer.cpp
> index
>
e58a6d3e13ce9fc0f660e9b901901bc1a7caf0f9..933dc22b10cc35f38f7aa478323ac9bd4e5e46f3
> 100644
> --- a/Source/core/rendering/FastTextAutosizer.cpp
> +++ b/Source/core/rendering/FastTextAutosizer.cpp
> @@ -176,7 +176,7 @@ void FastTextAutosizer::inflate(RenderBlock* block)
>  AtomicString FastTextAutosizer::fingerprint(const RenderBlock* block)
>  {
>      // FIXME(crbug.com/322340): Implement a better fingerprinting
> algorithm.
> -    return String::number((unsigned long long) block);
> +    return AtomicString::number((unsigned long long) block);
>  }
>
>  void FastTextAutosizer::recalcClusterIfNeeded(FastTextAutosizer::Cluster*
> cluster)
> Index: Source/core/rendering/HitTestResult.cpp
> diff --git a/Source/core/rendering/HitTestResult.cpp
> b/Source/core/rendering/HitTestResult.cpp
> index
>
ec14dec49af62504096d0151802d4d9dffcb9748..7e699e4095853b154655281e8e4774ba446f2cf1
> 100644
> --- a/Source/core/rendering/HitTestResult.cpp
> +++ b/Source/core/rendering/HitTestResult.cpp
> @@ -197,7 +197,7 @@ Frame* HitTestResult::targetFrame() const
>      if (!frame)
>          return 0;
>
> -    return frame->tree().find(m_innerURLElement->target());
> +    return frame->tree().find(AtomicString(m_innerURLElement->target()));
>  }
>
>  bool HitTestResult::isSelected() const
> Index: Source/core/rendering/RenderLayer.cpp
> diff --git a/Source/core/rendering/RenderLayer.cpp
> b/Source/core/rendering/RenderLayer.cpp
> index
>
bba59c16c35db7c94cd472514116501847df8a5b..ca657770c681d88c0836a817177405be6786cc44
> 100644
> --- a/Source/core/rendering/RenderLayer.cpp
> +++ b/Source/core/rendering/RenderLayer.cpp
> @@ -2041,7 +2041,7 @@ void RenderLayer::paintLayerContents(GraphicsContext*
> context, const LayerPainti
>              ReferenceClipPathOperation* referenceClipPathOperation =
> toReferenceClipPathOperation(style->clipPath());
>              Document& document = renderer()->document();
>              // FIXME: It doesn't work with forward or external SVG
> references (https://bugs.webkit.org/show_bug.cgi?id=90405)
> -            Element* element =
> document.getElementById(referenceClipPathOperation->fragment());
> +            Element* element =
>
document.getElementById(AtomicString(referenceClipPathOperation->fragment()));
>              if (element && element->hasTagName(SVGNames::clipPathTag) &&
> element->renderer()) {
>                  if (!rootRelativeBoundsComputed) {
>                      rootRelativeBounds =
> calculateLayerBounds(paintingInfo.rootLayer, &offsetFromRoot, 0);
> Index: Source/core/rendering/RenderLayerFilterInfo.cpp
> diff --git a/Source/core/rendering/RenderLayerFilterInfo.cpp
> b/Source/core/rendering/RenderLayerFilterInfo.cpp
> index
>
600ced9e77a16fbed2f811146f6566429667384f..84dbb252a8b787aaed4fead4042bf026e50cfddf
> 100644
> --- a/Source/core/rendering/RenderLayerFilterInfo.cpp
> +++ b/Source/core/rendering/RenderLayerFilterInfo.cpp
> @@ -129,7 +129,7 @@ void
> RenderLayerFilterInfo::updateReferenceFilterClients(const FilterOperations&
>          } else {
>              // Reference is internal; add layer as a client so we can
> trigger
>              // filter repaint on SVG attribute change.
> -            Element* filter =
>
m_layer->renderer()->node()->document().getElementById(referenceFilterOperation->fragment());
> +            Element* filter =
>
m_layer->renderer()->node()->document().getElementById(AtomicString(referenceFilterOperation->fragment()));
>              if (!filter || !filter->hasTagName(SVGNames::filterTag))
>                  continue;
>              if (filter->renderer())
> Index: Source/core/rendering/RenderThemeChromiumFontProvider.cpp
> diff --git a/Source/core/rendering/RenderThemeChromiumFontProvider.cpp
> b/Source/core/rendering/RenderThemeChromiumFontProvider.cpp
> index
>
527f8558f78d5f2dd688b2322ef2660d0ced68ea..b8083e015fe6a5bbfde774da684954c0f309f9d0
> 100644
> --- a/Source/core/rendering/RenderThemeChromiumFontProvider.cpp
> +++ b/Source/core/rendering/RenderThemeChromiumFontProvider.cpp
> @@ -46,9 +46,9 @@ float RenderThemeChromiumFontProvider::s_defaultFontSize =
> 16.0;
>  // FIXME: The only case where we know we don't match IE is for ANSI
> encodings.
>  // IE uses MS Shell Dlg there, which we render incorrectly at certain pixel
>  // sizes (e.g. 15px). So, for now we just use Arial.
> -const String& RenderThemeChromiumFontProvider::defaultGUIFont()
> +const AtomicString& RenderThemeChromiumFontProvider::defaultGUIFont()
>  {
> -    DEFINE_STATIC_LOCAL(String, fontFace, ("Arial"));
> +    DEFINE_STATIC_LOCAL(const AtomicString, fontFace, ("Arial",
> AtomicString::ConstructFromLiteral));
>      return fontFace;
>  }
>
> Index: Source/core/rendering/RenderThemeChromiumFontProvider.h
> diff --git a/Source/core/rendering/RenderThemeChromiumFontProvider.h
> b/Source/core/rendering/RenderThemeChromiumFontProvider.h
> index
>
57589c2a397d5859eacabf8e9006b8a67815bd43..f6ce1dfd361f60bc575fd830606877de7b434c58
> 100644
> --- a/Source/core/rendering/RenderThemeChromiumFontProvider.h
> +++ b/Source/core/rendering/RenderThemeChromiumFontProvider.h
> @@ -42,7 +42,7 @@ public:
>      static void setDefaultFontSize(int);
>
>  protected:
> -    static const WTF::String& defaultGUIFont();
> +    static const WTF::AtomicString& defaultGUIFont();
>
>      static float s_defaultFontSize;
>  };
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[Inactive, 2014-01-01 16:21:14]

On 2013/12/31 23:13:21, eseidel wrote:
> I suspect that framgent() should return an AtomicString?  Otherwise lgtm.

Right, I updated fragment() method in the patch for svg/ to return an
AtomicString and updated this patch accordingly.

[commit-bot: I haz the power, 2014-01-01 16:21:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ch.dumez@samsung.com/122943003/130001

[commit-bot: I haz the power, 2014-01-01 17:30:18]

Change committed as 164413