Bug 66062

Created attachment 103628 [details] Patch

(In reply to comment #0) > Platform PopupMenu Code Should Have Access To transformed bounding box of RenderMenuList This touches a lot of platform-specific code. If there's a nicer way to pass this information along to platform code, then please offer suggestions. We may use this information in certain Chromium platforms.

Attachment 103628 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Traceback (most recent call last): File "Tools/Scripts/check-webkit-style", line 135, in <module> main() File "Tools/Scripts/check-webkit-style", line 122, in main patch_checker.check(patch) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/patchreader.py", line 66, in check self._text_file_reader.process_file(file_path=path, line_numbers=line_numbers) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filereader.py", line 128, in process_file self._processor.process(lines, file_path, **kwargs) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 800, in process checker.check(lines) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 3556, in check self.handle_style_error, self.min_confidence) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 3432, in _process_lines include_state, function_state, class_state, file_state, error) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 3397, in process_line check_style(clean_lines, line, file_extension, class_state, file_state, error) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 2521, in check_style check_spacing(file_extension, clean_lines, line_number, error) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 1903, in check_spacing error(line_number, 'whitespace/braces', 5, 'Missing space inside { }.') File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/error_handlers.py", line 145, in __call__ file_path=self._file_path): File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 643, in is_reportable return self._filter_configuration.should_check(category, file_path) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filter.py", line 277, in should_check return self._filter_from_path(path).should_check(category) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filter.py", line 240, in _filter_from_path path_rules = self._path_rules_from_path(path) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filter.py", line 212, in _path_rules_from_path for (sub_paths, path_rules) in self._get_path_specific_lower(): File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filter.py", line 200, in _get_path_specific_lower sub_paths = map(str.lower, sub_paths) TypeError: descriptor 'lower' requires a 'str' object but received a 'tuple' If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #3) > Attachment 103628 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 > > Traceback (most recent call last): > File "Tools/Scripts/check-webkit-style", line 135, in <module> > main() > File "Tools/Scripts/check-webkit-style", line 122, in main > patch_checker.check(patch) > File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/patchreader.py", line 66, in check > self._text_file_reader.process_file(file_path=path, line_numbers=line_numbers) > File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filereader.py", line 128, in process_file > self._processor.process(lines, file_path, **kwargs) > File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 800, in process > checker.check(lines) > File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 3556, in check > self.handle_style_error, self.min_confidence) > File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 3432, in _process_lines > include_state, function_state, class_state, file_state, error) > File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 3397, in process_line > check_style(clean_lines, line, file_extension, class_state, file_state, error) > File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 2521, in check_style > check_spacing(file_extension, clean_lines, line_number, error) > File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 1903, in check_spacing > error(line_number, 'whitespace/braces', 5, 'Missing space inside { }.') > File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/error_handlers.py", line 145, in __call__ > file_path=self._file_path): > File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 643, in is_reportable > return self._filter_configuration.should_check(category, file_path) > File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filter.py", line 277, in should_check > return self._filter_from_path(path).should_check(category) > File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filter.py", line 240, in _filter_from_path > path_rules = self._path_rules_from_path(path) > File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filter.py", line 212, in _path_rules_from_path > for (sub_paths, path_rules) in self._get_path_specific_lower(): > File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filter.py", line 200, in _get_path_specific_lower > sub_paths = map(str.lower, sub_paths) > TypeError: descriptor 'lower' requires a 'str' object but received a 'tuple' > > > If any of these errors are false positives, please file a bug against check-webkit-style. This looks like a broken script :(

Comment on attachment 103628 [details] Patch Attachment 103628 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9357247

Created attachment 103632 [details] Patch

Attachment 103632 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Traceback (most recent call last): File "Tools/Scripts/check-webkit-style", line 135, in <module> main() File "Tools/Scripts/check-webkit-style", line 122, in main patch_checker.check(patch) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/patchreader.py", line 66, in check self._text_file_reader.process_file(file_path=path, line_numbers=line_numbers) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filereader.py", line 128, in process_file self._processor.process(lines, file_path, **kwargs) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 800, in process checker.check(lines) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 3556, in check self.handle_style_error, self.min_confidence) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 3432, in _process_lines include_state, function_state, class_state, file_state, error) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 3397, in process_line check_style(clean_lines, line, file_extension, class_state, file_state, error) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 2521, in check_style check_spacing(file_extension, clean_lines, line_number, error) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checkers/cpp.py", line 1903, in check_spacing error(line_number, 'whitespace/braces', 5, 'Missing space inside { }.') File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/error_handlers.py", line 145, in __call__ file_path=self._file_path): File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/checker.py", line 643, in is_reportable return self._filter_configuration.should_check(category, file_path) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filter.py", line 277, in should_check return self._filter_from_path(path).should_check(category) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filter.py", line 240, in _filter_from_path path_rules = self._path_rules_from_path(path) File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filter.py", line 212, in _path_rules_from_path for (sub_paths, path_rules) in self._get_path_specific_lower(): File "/mnt/git/webkit-style-queue/Tools/Scripts/webkitpy/style/filter.py", line 200, in _get_path_specific_lower sub_paths = map(str.lower, sub_paths) TypeError: descriptor 'lower' requires a 'str' object but received a 'tuple' If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 103632 [details] Patch Attachment 103632 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9371064

Comment on attachment 103632 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=103632&action=review > Source/WebCore/ChangeLog:3 > + Platform PopupMenu Code Should Have Access To transformed bounding box of RenderMenuList i don't really understand what a "transformed bounding box of RenderMenuList" is. can you explain that to me? > Source/WebCore/platform/PopupMenu.h:34 > + virtual void show(const IntRect&, const IntRect&, FrameView*, int index) = 0; It is extremely non-obvious what these two IntRect parameters mean. When it is not obvious, they should have a parameter name.

(In reply to comment #9) > (From update of attachment 103632 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=103632&action=review > > > Source/WebCore/ChangeLog:3 > > + Platform PopupMenu Code Should Have Access To transformed bounding box of RenderMenuList > > i don't really understand what a "transformed bounding box of RenderMenuList" is. can you explain that to me? > > > Source/WebCore/platform/PopupMenu.h:34 > > + virtual void show(const IntRect&, const IntRect&, FrameView*, int index) = 0; > > It is extremely non-obvious what these two IntRect parameters mean. When it is not obvious, they should have a parameter name. Let me be very specific about the desired functionality here: Ultimately we want the page scale factor to be taken into account when positioning popups on certain Chromium platforms. To do that, the chromium platform layer needs to have access to the size of the combobox after the application of the scaling (and other) transforms.

Created attachment 104109 [details] Patch

Attachment 104109 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1 Source/WebCore/platform/gtk/PopupMenuGtk.h:38: The parameter name "rect" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/PopupMenu.h:34: The parameter name "rect" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/loader/EmptyClients.h:72: Missing space inside { }. [whitespace/braces] [5] Source/WebKit2/WebProcess/WebCoreSupport/WebPopupMenu.h:52: The parameter name "rect" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebKit/chromium/src/ExternalPopupMenu.h:59: The parameter name "rect" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/mac/PopupMenuMac.h:46: The parameter name "rect" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/efl/PopupMenuEfl.h:39: The parameter name "rect" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/brew/PopupMenuBrew.h:39: The parameter name "rect" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/haiku/PopupMenuHaiku.h:41: The parameter name "rect" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/wx/PopupMenuWx.h:48: The parameter name "rect" adds no information, so it should be removed. [readability/parameter_name] [5] Source/WebCore/platform/chromium/PopupMenuChromium.h:52: The parameter name "rect" adds no information, so it should be removed. [readability/parameter_name] [5] Total errors found: 11 in 28 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 104109 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=104109&action=review >> Source/WebCore/platform/PopupMenu.h:34 >> + virtual void show(const IntRect& rect, const IntRect& transformedRect, FrameView*, int index) = 0; > > The parameter name "rect" adds no information, so it should be removed. [readability/parameter_name] [5] The style bot is right. "rect" and "transformedRect" aren't really specific enough. It would be good for the names to indicate what coordinate space the rects are in. > Source/WebCore/rendering/RenderTextControlSingleLine.cpp:185 > - m_searchPopup->popupMenu()->show(absoluteBoundingBoxRect(true), document()->view(), -1); > + IntRect boundingBox = absoluteBoundingBoxRect(true); > + m_searchPopup->popupMenu()->show(boundingBox, boundingBox, document()->view(), -1); Why are you passing the same rect twice in this case?

(In reply to comment #13) > (From update of attachment 104109 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=104109&action=review > > >> Source/WebCore/platform/PopupMenu.h:34 > >> + virtual void show(const IntRect& rect, const IntRect& transformedRect, FrameView*, int index) = 0; > > > > The parameter name "rect" adds no information, so it should be removed. [readability/parameter_name] [5] > > The style bot is right. "rect" and "transformedRect" aren't really specific enough. It would be good for the names to indicate what coordinate space the rects are in. > > > Source/WebCore/rendering/RenderTextControlSingleLine.cpp:185 > > - m_searchPopup->popupMenu()->show(absoluteBoundingBoxRect(true), document()->view(), -1); > > + IntRect boundingBox = absoluteBoundingBoxRect(true); > > + m_searchPopup->popupMenu()->show(boundingBox, boundingBox, document()->view(), -1); > > Why are you passing the same rect twice in this case? I will fix the names now. As for RenderTextControlSingleLine...it already expected taking in the transformed rect..I am looking into making this more consistent now.

(In reply to comment #14) > (In reply to comment #13) > > (From update of attachment 104109 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=104109&action=review > > > > >> Source/WebCore/platform/PopupMenu.h:34 > > >> + virtual void show(const IntRect& rect, const IntRect& transformedRect, FrameView*, int index) = 0; > > > > > > The parameter name "rect" adds no information, so it should be removed. [readability/parameter_name] [5] > > > > The style bot is right. "rect" and "transformedRect" aren't really specific enough. It would be good for the names to indicate what coordinate space the rects are in. > > > > > Source/WebCore/rendering/RenderTextControlSingleLine.cpp:185 > > > - m_searchPopup->popupMenu()->show(absoluteBoundingBoxRect(true), document()->view(), -1); > > > + IntRect boundingBox = absoluteBoundingBoxRect(true); > > > + m_searchPopup->popupMenu()->show(boundingBox, boundingBox, document()->view(), -1); > > > > Why are you passing the same rect twice in this case? > > I will fix the names now. As for RenderTextControlSingleLine...it already expected taking in the transformed rect..I am looking into making this more consistent now. Are localRect, and absoluteRect sufficiently descriptive parameter names?

(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #13) > > > (From update of attachment 104109 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=104109&action=review > > > > > > >> Source/WebCore/platform/PopupMenu.h:34 > > > >> + virtual void show(const IntRect& rect, const IntRect& transformedRect, FrameView*, int index) = 0; > > > > > > > > The parameter name "rect" adds no information, so it should be removed. [readability/parameter_name] [5] > > > > > > The style bot is right. "rect" and "transformedRect" aren't really specific enough. It would be good for the names to indicate what coordinate space the rects are in. > > > > > > > Source/WebCore/rendering/RenderTextControlSingleLine.cpp:185 > > > > - m_searchPopup->popupMenu()->show(absoluteBoundingBoxRect(true), document()->view(), -1); > > > > + IntRect boundingBox = absoluteBoundingBoxRect(true); > > > > + m_searchPopup->popupMenu()->show(boundingBox, boundingBox, document()->view(), -1); > > > > > > Why are you passing the same rect twice in this case? > > > > I will fix the names now. As for RenderTextControlSingleLine...it already expected taking in the transformed rect..I am looking into making this more consistent now. > > Are localRect, and absoluteRect sufficiently descriptive parameter names? Let me step back and ask another question, is there any reason why any platform would not want the transformed rect of the combo box? Let me be specific about why we want this. We want a combo box popup to appear below a combo box even if the page is scaled up/down. Thus, we need to have the scaled height of the box.

Created attachment 106644 [details] Popup mispositioned when page scaled up (should be underneath select element)

Created attachment 106650 [details] Patch

Created attachment 106792 [details] Patch

Comment on attachment 106792 [details] Patch Seems like we need to scale the font size, too, and any other values the PopupMenu gets from its client.

(In reply to comment #20) > (From update of attachment 106792 [details]) > Seems like we need to scale the font size, too, and any other values the PopupMenu gets from its client. Font size is already scaled. I believe PopupMenuStyles are all scaled already.

Comment on attachment 106792 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106792&action=review > Source/WebCore/ChangeLog:4 > + Platform code should take in the transformed rect of the select element > + https://bugs.webkit.org/show_bug.cgi?id=66062 Can you please retitle the bug to describe a user-visible symptom? > Source/WebCore/ChangeLog:11 > + (WebCore::RenderMenuList::showPopup): This change needs some explanation. Just adding "true" at the callsite provides no meaning for reviewers or other folks who look back at this change later. Why don't we have to change a function like showPopup for the Recent Searches menu? > Source/WebCore/ChangeLog:16 > + (WebCore::RenderMenuList::clientPaddingLeft): > + (WebCore::RenderMenuList::clientPaddingRight): > + * rendering/RenderTextControlSingleLine.cpp: > + (WebCore::RenderTextControlSingleLine::clientPaddingLeft): > + (WebCore::RenderTextControlSingleLine::clientPaddingRight): These changes should also have some comments.

Comment on attachment 106792 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106792&action=review >> Source/WebCore/ChangeLog:4 >> + https://bugs.webkit.org/show_bug.cgi?id=66062 > > Can you please retitle the bug to describe a user-visible symptom? Done >> Source/WebCore/ChangeLog:11 >> + (WebCore::RenderMenuList::showPopup): > > This change needs some explanation. Just adding "true" at the callsite provides no meaning for reviewers or other folks who look back at this change later. > > Why don't we have to change a function like showPopup for the Recent Searches menu? Done and it already does apparently. >> Source/WebCore/ChangeLog:16 >> + (WebCore::RenderTextControlSingleLine::clientPaddingRight): > > These changes should also have some comments. Done.

Created attachment 106805 [details] Patch

(In reply to comment #23) > (From update of attachment 106792 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106792&action=review > > >> Source/WebCore/ChangeLog:4 > >> + https://bugs.webkit.org/show_bug.cgi?id=66062 > > > > Can you please retitle the bug to describe a user-visible symptom? > > Done The current title still isn't a user-visible symptom. A user-visible symptom would be something like "<select> popup menu appears in wrong place when page is zoomed". That's something an actual user can witness.

Comment on attachment 106805 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106805&action=review > Source/WebCore/rendering/RenderMenuList.cpp:306 > // Compute the top left taking transforms into account, but use > // the actual width of the element to size the popup. > FloatPoint absTopLeft = localToAbsolute(FloatPoint(), false, true); > - LayoutRect absBounds = absoluteBoundingBoxRect(); > + // Compute the bounds taking transforms into account. > + LayoutRect absBounds = absoluteBoundingBoxRect(true); The comment above yours seems to say that we were intentionally not passing "true" here. Do you know why that decision was made originally? You need to update that comment so it agrees with the current code. > Source/WebCore/rendering/RenderMenuList.cpp:516 > + // The padding in the popup box should scale with the pageScaleFactor. > + return leftPadding * frame()->pageScaleFactor(); This comment is just restating what the code says. We typically try to avoid those kinds of comments, as they don't add any information but increase the maintenance burden of the code. It's also very easy for them to become inaccurate as the code changes. I don't think you need a comment in these functions. But I do think it would be good to put explanations in the ChangeLog of your changes. Here's a good example ChangeLog entry: <http://trac.webkit.org/changeset/92556/trunk/Source/WebCore/ChangeLog>. Ditto for all the other client functions.

(In reply to comment #23) > (From update of attachment 106792 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106792&action=review > > >> Source/WebCore/ChangeLog:11 > >> + (WebCore::RenderMenuList::showPopup): > > > > This change needs some explanation. Just adding "true" at the callsite provides no meaning for reviewers or other folks who look back at this change later. > > > > Why don't we have to change a function like showPopup for the Recent Searches menu? > > Done and it already does apparently. You should mention in your ChangeLog that the Recent Searches menu already does this. Then reviewers won't think you forgot to change it.

Comment on attachment 106805 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106805&action=review >> Source/WebCore/rendering/RenderMenuList.cpp:306 >> + LayoutRect absBounds = absoluteBoundingBoxRect(true); > > The comment above yours seems to say that we were intentionally not passing "true" here. Do you know why that decision was made originally? You need to update that comment so it agrees with the current code. I'm not sure. It looks like Simon Fraser wrote that line for https://bugs.webkit.org/show_bug.cgi?id=15678. I'll ask him to take a look at this patch as well. >> Source/WebCore/rendering/RenderMenuList.cpp:516 >> + return leftPadding * frame()->pageScaleFactor(); > > This comment is just restating what the code says. We typically try to avoid those kinds of comments, as they don't add any information but increase the maintenance burden of the code. It's also very easy for them to become inaccurate as the code changes. I don't think you need a comment in these functions. But I do think it would be good to put explanations in the ChangeLog of your changes. Here's a good example ChangeLog entry: <http://trac.webkit.org/changeset/92556/trunk/Source/WebCore/ChangeLog>. > > Ditto for all the other client functions. Fixing this now, thanks.

Comment on attachment 106805 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106805&action=review >>> Source/WebCore/rendering/RenderMenuList.cpp:306 >>> + LayoutRect absBounds = absoluteBoundingBoxRect(true); >> >> The comment above yours seems to say that we were intentionally not passing "true" here. Do you know why that decision was made originally? You need to update that comment so it agrees with the current code. > > I'm not sure. It looks like Simon Fraser wrote that line for https://bugs.webkit.org/show_bug.cgi?id=15678. I'll ask him to take a look at this patch as well. It was probably to keep the popup a reasonable size if the transform was a rotation. I think the right approach is to use absoluteBoundingBoxRect(true), then set the width of the popup to the untransformed width of the renderer * pageScaleFactor. Would be good to have some testcases here.

Created attachment 106881 [details] Patch

Comment on attachment 106881 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106881&action=review > Source/WebCore/ChangeLog:8 > + No new tests because popups are very platform-dependent. That is not a good reason to leave a bug fix untested.

Comment on attachment 106881 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106881&action=review >> Source/WebCore/ChangeLog:8 >> + No new tests because popups are very platform-dependent. > > That is not a good reason to leave a bug fix untested. I agree it makes it harder to write a test, but we need tests for bug fixes.

(In reply to comment #32) > (From update of attachment 106881 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106881&action=review > > >> Source/WebCore/ChangeLog:8 > >> + No new tests because popups are very platform-dependent. > > > > That is not a good reason to leave a bug fix untested. > > I agree it makes it harder to write a test, but we need tests for bug fixes. Can popups be tested in a layout test [I haven't tried yet]?

(In reply to comment #33) > (In reply to comment #32) > > (From update of attachment 106881 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=106881&action=review > > > > >> Source/WebCore/ChangeLog:8 > > >> + No new tests because popups are very platform-dependent. > > > > > > That is not a good reason to leave a bug fix untested. > > > > I agree it makes it harder to write a test, but we need tests for bug fixes. > > Can popups be tested in a layout test [I haven't tried yet]? So I can't test popups in layout tests. I'd love to upstream this fix as this is a real issue on certain Chromium platforms but I just don't know how to produce an automated test for it. The current layout test infrastructure does not allow for this kind of test. Could someone more experienced than I please give me suggestions?

I can see window.internals test for this -- it should just query coordinates, and see if we get the right ones. WDYT?

(In reply to comment #35) > I can see window.internals test for this -- it should just query coordinates, and see if we get the right ones. WDYT? I shall attempt this thanks.

Comment on attachment 106881 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=106881&action=review > Source/WebCore/rendering/RenderTextControlSingleLine.cpp:639 > - return padding; > + return padding * frame()->pageScaleFactor(); Really? I thought we applied the pageScaleFactor at only a very few high-level places? Are you sure all these low-level rendering objects need to know about it? If so, how do we find all the places it's needed?