Bug 38073

Created attachment 54208 [details] Patch

Comment on attachment 54208 [details] Patch What compiler generates these warnings?

Comment on attachment 54208 [details] Patch > class StyleReflection; > class StyleTransformData; > struct ContentData; > -struct ShadowData; > +class ShadowData; This should be re-sorted above with the class forward declarations. And the struct forward declarations should be in a separate paragraph (blank line between the paragraphs).

Comment on attachment 54208 [details] Patch > - unsigned width() const { return m_width; } > + unsigned width() const { return static_cast<unsigned>(m_width); } > void setWidth(float w) { m_width = w; } This worries me; not the type cast, but the existing code. Why is it OK to convert the width into an unsigned?

Created attachment 54330 [details] Patch

The warnings are from MSVC, but they are all fairly general. I agree that the TextMetrics rounding looks weird, but I'm not familiar enough with this code to guess whether or not it's an issue.

(In reply to comment #6) > The warnings are from MSVC Good to know. Are these particular MSVC warnings off for the Windows build (the one Apple uses for Windows Safari)? Should we turn them on? > but they are all fairly general. I don't know what that means. > I agree that the > TextMetrics rounding looks weird, but I'm not familiar enough with this code to > guess whether or not it's an issue. Generally I think it’s not good to silence a warning if the warning may be pointed to a real problem. I am familiar enough to know that it might be an issue.

Comment on attachment 54330 [details] Patch I recommend posting and landing the uncontentious fixes first, and then asking mitz about what's up with the width() code.

Created attachment 54370 [details] Patch

I like Eric's idea, this patch does that. It adds a FIXME instead of the cast.

(In reply to comment #7) > Are these particular MSVC warnings off for the Windows build (the one Apple > uses for Windows Safari)? Should we turn them on? I'm still waiting for the answer to this question.

I should have clarified that these are warnings seen in the Chromium Windows build using those warning settings. I don't know which warnings the Safari Win port suppresses and do not have access to a Safari Win build environment to check. I think those decisions will have to be made by the Safari Win port maintainers. That said, I think it's reasonable that shared code not generate warnings for any port unless there's a good reason to (which there does not seem to be in this case).

s/Safari/WebKit/ (In reply to comment #12) > I should have clarified that these are warnings seen in the Chromium Windows > build using those warning settings. I don't know which warnings the Safari Win > port suppresses and do not have access to a Safari Win build environment to > check. Here ya go: http://webkit.org/building/checkout.html > I think those decisions will have to be made by the Safari Win port > maintainers. I bet folks would be ok with the warnings being turned on as long as it doesn't break the build. If in doubt, you could send email to webkit-dev asking for objections. By turning on the warning, you will help make sure that more people are concerned about them as well.

Comment on attachment 54370 [details] Patch LGTM.

Adam, Steve, and Brian are a few of the windows guys. If you post a patch to WebCore.vcproj enabling the warnings, I expect they'd be happy to review.

Attachment 54370 [details] was posted by a committer and has review+, assigning to James Robinson for commit.

Comment on attachment 54370 [details] Patch Clearing flags on attachment: 54370 Committed r58718: <http://trac.webkit.org/changeset/58718>

All reviewed patches have been landed. Closing bug.

I filed bug 51566 for the discussion of the mysterious truncator unsigned width() from comments #4, #7, #8, #9.