Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
685153003:
Pointer/hover media query support: platform-independent changes (Closed)
|
Created:
6 years, 1 month ago by mustaq Modified:
6 years, 1 month ago CC:
chromium-reviews, mkwst+moarreviews-renderer_chromium.org, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionPointer/hover media query support: platform-independent changes
Platform-independent changes on our way to support pointer, any-pointer, hover and any-hover media queries.
http://dev.w3.org/csswg/mediaqueries-4/#mf-interaction
This will be followed by platform-specific changes. For example, crrev.com/696713002 adds Android-specific changes this CL.
BUG=136119
Committed: https://crrev.com/29eb70994876255e91e1d2400843ede7436ce9d0
Cr-Commit-Position: refs/heads/master@{#304126}
Patch Set 1 #Patch Set 2 : Added IPC (de)serialization macros #Patch Set 3 : Removed platform-dependent changes, and rebased #
Total comments: 2
Patch Set 4 : Rebased, nuked content::{Pointer,Hover}Type #
Total comments: 3
Patch Set 5 : Fixed enum naming style #
Total comments: 4
Patch Set 6 : #
Messages
Total messages: 28 (5 generated)
mustaq@chromium.org changed reviewers: + jdduke@chromium.org, rbyers@chromium.org
ptal.
On 2014/10/31 15:33:44, mustaq wrote: > ptal. One minor note. When you change the patch description, you'll want to keep the logical title as the first line, as the description text is the only text that follows the patch into git.
On 2014/10/31 15:47:45, jdduke wrote: > On 2014/10/31 15:33:44, mustaq wrote: > > ptal. > > One minor note. When you change the patch description, you'll want to keep the > logical title as the first line, as the description text is the only text that > follows the patch into git. Done
LGTM also Note that you won't be able to rely on the IPC validation system much here (sicne the flags are defines as arbitrary ints). But this is really only a concern if we were reading the values in the browser process (to protect browser code against malicous code running in the renderer). Have you come across any other examples of using bit-fields in IPC messages? I could imagine someone adding a custom validator function to reject IPCs messages with invalid bitfield values. But I don't imagine these types would ever from flow renderer to browser, so I think it's not very relevant here.
https://codereview.chromium.org/685153003/diff/40001/content/public/common/we... File content/public/common/web_preferences.h (right): https://codereview.chromium.org/685153003/diff/40001/content/public/common/we... content/public/common/web_preferences.h:45: POINTER_TYPE_NONE = 1, So just to be clear, are we saying there's no benefit in referencing these enums from ui, instead duplicating theme here? I can understand duplicated copies between blink/content, but between ui/blink/content it's getting rather unfortunate.
On 2014/11/11 01:17:54, jdduke wrote: > https://codereview.chromium.org/685153003/diff/40001/content/public/common/we... > File content/public/common/web_preferences.h (right): > > https://codereview.chromium.org/685153003/diff/40001/content/public/common/we... > content/public/common/web_preferences.h:45: POINTER_TYPE_NONE = 1, > So just to be clear, are we saying there's no benefit in referencing these enums > from ui, instead duplicating theme here? I can understand duplicated copies > between blink/content, but between ui/blink/content it's getting rather > unfortunate. No, we will instead use the ui types here instead. I had created a patch yesterday that seemed fine, I'll clean it up now before uploading here.
ptal https://codereview.chromium.org/685153003/diff/40001/content/public/common/we... File content/public/common/web_preferences.h (right): https://codereview.chromium.org/685153003/diff/40001/content/public/common/we... content/public/common/web_preferences.h:45: POINTER_TYPE_NONE = 1, On 2014/11/11 01:17:54, jdduke wrote: > So just to be clear, are we saying there's no benefit in referencing these enums > from ui, instead duplicating theme here? I can understand duplicated copies > between blink/content, but between ui/blink/content it's getting rather > unfortunate. Done removing the types from here. https://codereview.chromium.org/685153003/diff/60001/content/public/common/we... File content/public/common/web_preferences.cc (right): https://codereview.chromium.org/685153003/diff/60001/content/public/common/we... content/public/common/web_preferences.cc:55: COMPILE_ASSERT_MATCHING_ENUMS(ui::PointerTypeNone, Not sure what's the best place for these checks. Any suggestion?
Thanks. https://codereview.chromium.org/685153003/diff/60001/ui/base/touch/touch_devi... File ui/base/touch/touch_device.h (right): https://codereview.chromium.org/685153003/diff/60001/ui/base/touch/touch_devi... ui/base/touch/touch_device.h:35: PointerTypeFirst = PointerTypeNone, I think these comments are slightly out-of-date, and we'll still want to static assert that they match the blink counterparts (also I think you're missing the gyp/gn additions for the Java enum generation?).
Patchset #5 (id:80001) has been deleted
https://codereview.chromium.org/685153003/diff/60001/ui/base/touch/touch_devi... File ui/base/touch/touch_device.h (right): https://codereview.chromium.org/685153003/diff/60001/ui/base/touch/touch_devi... ui/base/touch/touch_device.h:35: PointerTypeFirst = PointerTypeNone, On 2014/11/11 15:59:30, jdduke wrote: > I think these comments are slightly out-of-date, and we'll still want to static > assert that they match the blink counterparts (also I think you're missing the > gyp/gn additions for the Java enum generation?). - Updated the comments. - Removed the CPP-Java-enum directives completely. Will add them plus the build rules in the other (follow-up, platform-specific) CL. - Fixed enum naming style as per mkosiba's comment in the other CL.
Looks good, but you'll need owners.
mustaq@chromium.org changed reviewers: + nasko@chromium.org, sadrul@chromium.org
Need owners' approval. nasko@chromium.org: Please review changes in content/public/common/common_param_traits_macros.h content/public/common/web_preferences.cc content/public/common/web_preferences.h content/renderer/render_view_impl.cc sadrul@chromium.org: Please review changes in ui/base/touch/touch_device.h
https://codereview.chromium.org/685153003/diff/100001/ui/base/touch/touch_dev... File ui/base/touch/touch_device.h (right): https://codereview.chromium.org/685153003/diff/100001/ui/base/touch/touch_dev... ui/base/touch/touch_device.h:49: }; I was going to suggest using the WebSettings::PointerType enums in WebPreferences, since these new enums aren't used anywhere in UI code. But looks like there will be follow-up patches that uses these on all platforms? In that case, LGTM
On 2014/11/11 18:40:50, sadrul wrote: > https://codereview.chromium.org/685153003/diff/100001/ui/base/touch/touch_dev... > File ui/base/touch/touch_device.h (right): > > https://codereview.chromium.org/685153003/diff/100001/ui/base/touch/touch_dev... > ui/base/touch/touch_device.h:49: }; > I was going to suggest using the WebSettings::PointerType enums in > WebPreferences, since these new enums aren't used anywhere in UI code. But looks > like there will be follow-up patches that uses these on all platforms? In that > case, LGTM Yes, all of ui/base/touch/touch_device*.cc will be using the enums.
LGTM, provided the comment is addressed. https://codereview.chromium.org/685153003/diff/100001/content/public/common/c... File content/public/common/common_param_traits_macros.h (right): https://codereview.chromium.org/685153003/diff/100001/content/public/common/c... content/public/common/common_param_traits_macros.h:49: IPC_ENUM_TRAITS_MIN_MAX_VALUE(ui::PointerType, Since these enums are bitfields, no need for this macro.
https://codereview.chromium.org/685153003/diff/100001/content/public/common/c... File content/public/common/common_param_traits_macros.h (right): https://codereview.chromium.org/685153003/diff/100001/content/public/common/c... content/public/common/common_param_traits_macros.h:49: IPC_ENUM_TRAITS_MIN_MAX_VALUE(ui::PointerType, On 2014/11/12 22:07:00, nasko wrote: > Since these enums are bitfields, no need for this macro. Done.
https://codereview.chromium.org/685153003/diff/100001/content/public/common/c... File content/public/common/common_param_traits_macros.h (right): https://codereview.chromium.org/685153003/diff/100001/content/public/common/c... content/public/common/common_param_traits_macros.h:49: IPC_ENUM_TRAITS_MIN_MAX_VALUE(ui::PointerType, On 2014/11/12 22:12:58, mustaq wrote: > On 2014/11/12 22:07:00, nasko wrote: > > Since these enums are bitfields, no need for this macro. > > Done. I had to bring back these range-checking macros! Looks like the code doesn't compile without them. I guess the (de)serialization code relies on them?
On 2014/11/13 16:02:05, mustaq wrote: > https://codereview.chromium.org/685153003/diff/100001/content/public/common/c... > File content/public/common/common_param_traits_macros.h (right): > > https://codereview.chromium.org/685153003/diff/100001/content/public/common/c... > content/public/common/common_param_traits_macros.h:49: > IPC_ENUM_TRAITS_MIN_MAX_VALUE(ui::PointerType, > On 2014/11/12 22:12:58, mustaq wrote: > > On 2014/11/12 22:07:00, nasko wrote: > > > Since these enums are bitfields, no need for this macro. > > > > Done. > > I had to bring back these range-checking macros! Looks like the code doesn't > compile without them. I guess the (de)serialization code relies on them? You need some kind of macro to inform the serializer, of which the range-checking are but one flavor (I think there's just an ENUM_TRAITS variant without range-checking).
On 2014/11/12 22:07:00, nasko wrote: > LGTM, provided the comment is addressed. > > https://codereview.chromium.org/685153003/diff/100001/content/public/common/c... > File content/public/common/common_param_traits_macros.h (right): > > https://codereview.chromium.org/685153003/diff/100001/content/public/common/c... > content/public/common/common_param_traits_macros.h:49: > IPC_ENUM_TRAITS_MIN_MAX_VALUE(ui::PointerType, > Since these enums are bitfields, no need for this macro. Note that the enums are used in two ways: as bit fields for "what devices exist" (which are stored as 'int' in the message), and as a single value for "what is the primary device" which is stored as the enum. The range check is used and makes some sense for the latter (though doesn't guard against all invalid values), but not the former. It wasn't clear to me what the best thing to do was - I thought what Mustaq had was reasonable, but avoiding confusion by removing any range checking at all also sounds like a good idea to me (especially since we have no plans to ever send values of this type from the renderer to the browser).
On 2014/11/13 16:15:10, Rick Byers wrote: > On 2014/11/12 22:07:00, nasko wrote: > > LGTM, provided the comment is addressed. > > > > > https://codereview.chromium.org/685153003/diff/100001/content/public/common/c... > > File content/public/common/common_param_traits_macros.h (right): > > > > > https://codereview.chromium.org/685153003/diff/100001/content/public/common/c... > > content/public/common/common_param_traits_macros.h:49: > > IPC_ENUM_TRAITS_MIN_MAX_VALUE(ui::PointerType, > > Since these enums are bitfields, no need for this macro. > > Note that the enums are used in two ways: as bit fields for "what devices exist" > (which are stored as 'int' in the message), and as a single value for "what is > the primary device" which is stored as the enum. The range check is used and > makes some sense for the latter (though doesn't guard against all invalid > values), but not the former. It wasn't clear to me what the best thing to do > was - I thought what Mustaq had was reasonable, but avoiding confusion by > removing any range checking at all also sounds like a good idea to me > (especially since we have no plans to ever send values of this type from the > renderer to the browser). The IPC_ENUM_TRAITS() marco works but is deprecated (presubmit warning). Our Security Tips page suggest the min-max macros instead: http://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc
On 2014/11/13 20:59:20, mustaq wrote: > On 2014/11/13 16:15:10, Rick Byers wrote: > > Note that the enums are used in two ways: as bit fields for "what devices > exist" > > (which are stored as 'int' in the message), and as a single value for "what is > > the primary device" which is stored as the enum. The range check is used and > > makes some sense for the latter (though doesn't guard against all invalid > > values), but not the former. It wasn't clear to me what the best thing to do > > was - I thought what Mustaq had was reasonable, but avoiding confusion by > > removing any range checking at all also sounds like a good idea to me > > (especially since we have no plans to ever send values of this type from the > > renderer to the browser). > > The IPC_ENUM_TRAITS() marco works but is deprecated (presubmit warning). Our > Security Tips page suggest the min-max macros instead: > http://www.chromium.org/Home/chromium-security/education/security-tips-for-ipc Yeah, even as bit fields I don't see how the bounds checking hurts, in fact, it would seem just as valid.
Patchset #6 (id:120001) has been deleted
The CQ bit was checked by mustaq@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/685153003/140001
Message was sent while issue was closed.
Committed patchset #6 (id:140001)
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/29eb70994876255e91e1d2400843ede7436ce9d0 Cr-Commit-Position: refs/heads/master@{#304126} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
