Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11725005:
Checks the possibility of overscans from EDID extension data. (Closed)
|
Created:
7 years, 11 months ago by Jun Mukai Modified:
7 years, 11 months ago CC:
chromium-reviews, sadrul, yusukes+watch_chromium.org, derat+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionChecks the possibility of overscans from EDID extension data.
BUG=141005
TEST=ui_unittests --gtest_filter='X11UtilTest.*' passed
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=175421
Patch Set 1 #Patch Set 2 : #
Total comments: 10
Patch Set 3 : fix / adding x11_util_unittest.cc #
Total comments: 12
Patch Set 4 : #
Total comments: 2
Patch Set 5 : #
Total comments: 9
Patch Set 6 : #
Messages
Total messages: 20 (0 generated)
https://codereview.chromium.org/11725005/diff/2001/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/11725005/diff/2001/ui/base/x/x11_util.cc#newc... ui/base/x/x11_util.cc:334: bool GetEDIDProperty(XID output, unsigned long* nitems, unsigned char* prop) { nit: add a comment stating that the caller must free |prop| using XFree() https://codereview.chromium.org/11725005/diff/2001/ui/base/x/x11_util.cc#newc... ui/base/x/x11_util.cc:1381: bool GetOutputOverscanFlag(XID output) { since this code does a lot of tricky byte-reading, it would be better to move most of it into a separate ParseEDIDProperty() function and write unit tests for it. https://codereview.chromium.org/11725005/diff/2001/ui/base/x/x11_util.cc#newc... ui/base/x/x11_util.cc:1438: // Here doesn't care the difference between preferred video format / nit: reword this to something like: // The difference between preferred, IT, and CE video formats // doesn't matter since this tag is skipped. https://codereview.chromium.org/11725005/diff/2001/ui/base/x/x11_util.h File ui/base/x/x11_util.h (right): https://codereview.chromium.org/11725005/diff/2001/ui/base/x/x11_util.h#newco... ui/base/x/x11_util.h:281: UI_EXPORT bool GetOutputOverscanFlag(XID output); nit: s/Get/Has/
Drive-by https://codereview.chromium.org/11725005/diff/2001/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/11725005/diff/2001/ui/base/x/x11_util.cc#newc... ui/base/x/x11_util.cc:1295: if (!GetEDIDProperty(output, &nitems, prop)) This doesn't actually set the value of |prop|, does it? Shouldn't you need to send &prop?
https://codereview.chromium.org/11725005/diff/2001/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/11725005/diff/2001/ui/base/x/x11_util.cc#newc... ui/base/x/x11_util.cc:334: bool GetEDIDProperty(XID output, unsigned long* nitems, unsigned char* prop) { On 2013/01/03 19:04:19, Daniel Erat wrote: > nit: add a comment stating that the caller must free |prop| using XFree() Done. https://codereview.chromium.org/11725005/diff/2001/ui/base/x/x11_util.cc#newc... ui/base/x/x11_util.cc:1295: if (!GetEDIDProperty(output, &nitems, prop)) On 2013/01/03 19:08:32, sadrul wrote: > This doesn't actually set the value of |prop|, does it? Shouldn't you need to > send &prop? aww, right. Fixed. https://codereview.chromium.org/11725005/diff/2001/ui/base/x/x11_util.cc#newc... ui/base/x/x11_util.cc:1381: bool GetOutputOverscanFlag(XID output) { On 2013/01/03 19:04:19, Daniel Erat wrote: > since this code does a lot of tricky byte-reading, it would be better to move > most of it into a separate ParseEDIDProperty() function and write unit tests for > it. Added. https://codereview.chromium.org/11725005/diff/2001/ui/base/x/x11_util.cc#newc... ui/base/x/x11_util.cc:1438: // Here doesn't care the difference between preferred video format / On 2013/01/03 19:04:19, Daniel Erat wrote: > nit: reword this to something like: > > // The difference between preferred, IT, and CE video formats > // doesn't matter since this tag is skipped. Sorry, the comment location was wrong. Moved to the right position (below) and rephrased as you suggested. https://codereview.chromium.org/11725005/diff/2001/ui/base/x/x11_util.h File ui/base/x/x11_util.h (right): https://codereview.chromium.org/11725005/diff/2001/ui/base/x/x11_util.h#newco... ui/base/x/x11_util.h:281: UI_EXPORT bool GetOutputOverscanFlag(XID output); On 2013/01/03 19:04:19, Daniel Erat wrote: > nit: s/Get/Has/ Done.
https://codereview.chromium.org/11725005/diff/8001/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/11725005/diff/8001/ui/base/x/x11_util.cc#newc... ui/base/x/x11_util.cc:1408: if (nitems < kNumExtensionsOffset) { should this be <= instead? nit: remove curly brackets https://codereview.chromium.org/11725005/diff/8001/ui/base/x/x11_util.cc#newc... ui/base/x/x11_util.cc:1433: const unsigned char* extension = prop + kExtensionBase + i * kExtensionSize; don't you need to check that |prop| is long enough first? it seems like broken/malicious hardware could make you reach beyond the end otherwise. if i'm not confused about this, please add tests to make sure that we don't crash on broken input. https://codereview.chromium.org/11725005/diff/8001/ui/base/x/x11_util.cc#newc... ui/base/x/x11_util.cc:1442: // a data block is encoded as: nit: s/a /A / https://codereview.chromium.org/11725005/diff/8001/ui/base/x/x11_util.cc#newc... ui/base/x/x11_util.cc:1450: data_block += payload_length + 1; i think you need to check nitems here too https://codereview.chromium.org/11725005/diff/8001/ui/base/x/x11_util.h File ui/base/x/x11_util.h (right): https://codereview.chromium.org/11725005/diff/8001/ui/base/x/x11_util.h#newco... ui/base/x/x11_util.h:284: // Parse |prop| as EDID data and stores extracted data into |manufacturer_id|, nit: s/Parse/Parses/ https://codereview.chromium.org/11725005/diff/8001/ui/base/x/x11_util.h#newco... ui/base/x/x11_util.h:285: // |serial_number|, and |human_readable_name| and returns true. // This is nit: remove extra '//'
https://codereview.chromium.org/11725005/diff/8001/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/11725005/diff/8001/ui/base/x/x11_util.cc#newc... ui/base/x/x11_util.cc:1408: if (nitems < kNumExtensionsOffset) { On 2013/01/03 21:59:14, Daniel Erat wrote: > should this be <= instead? > > nit: remove curly brackets no, this function will access to prop[kNumExtensionsOffset] and the code check its safety. removed for curly brackets, and also moved below to look the code easier. https://codereview.chromium.org/11725005/diff/8001/ui/base/x/x11_util.cc#newc... ui/base/x/x11_util.cc:1433: const unsigned char* extension = prop + kExtensionBase + i * kExtensionSize; On 2013/01/03 21:59:14, Daniel Erat wrote: > don't you need to check that |prop| is long enough first? it seems like > broken/malicious hardware could make you reach beyond the end otherwise. > > if i'm not confused about this, please add tests to make sure that we don't > crash on broken input. You're right. Added tests too. https://codereview.chromium.org/11725005/diff/8001/ui/base/x/x11_util.cc#newc... ui/base/x/x11_util.cc:1442: // a data block is encoded as: On 2013/01/03 21:59:14, Daniel Erat wrote: > nit: s/a /A / Done. https://codereview.chromium.org/11725005/diff/8001/ui/base/x/x11_util.cc#newc... ui/base/x/x11_util.cc:1450: data_block += payload_length + 1; On 2013/01/03 21:59:14, Daniel Erat wrote: > i think you need to check nitems here too Done. https://codereview.chromium.org/11725005/diff/8001/ui/base/x/x11_util.h File ui/base/x/x11_util.h (right): https://codereview.chromium.org/11725005/diff/8001/ui/base/x/x11_util.h#newco... ui/base/x/x11_util.h:284: // Parse |prop| as EDID data and stores extracted data into |manufacturer_id|, On 2013/01/03 21:59:14, Daniel Erat wrote: > nit: s/Parse/Parses/ Done. https://codereview.chromium.org/11725005/diff/8001/ui/base/x/x11_util.h#newco... ui/base/x/x11_util.h:285: // |serial_number|, and |human_readable_name| and returns true. // This is On 2013/01/03 21:59:14, Daniel Erat wrote: > nit: remove extra '//' Done.
https://codereview.chromium.org/11725005/diff/11002/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/11725005/diff/11002/ui/base/x/x11_util.cc#new... ui/base/x/x11_util.cc:1426: if (nitems < kNumExtensionsOffset) it still looks to me like there's an off-by-one here. for nitems=126, this condition will be false and num_extensions will be assigned prop[126], i.e. one byte beyond the end of the array.
https://codereview.chromium.org/11725005/diff/11002/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/11725005/diff/11002/ui/base/x/x11_util.cc#new... ui/base/x/x11_util.cc:1426: if (nitems < kNumExtensionsOffset) On 2013/01/03 23:09:37, Daniel Erat wrote: > it still looks to me like there's an off-by-one here. for nitems=126, this > condition will be false and num_extensions will be assigned prop[126], i.e. one > byte beyond the end of the array. aww, right... thanks. Added test too.
https://codereview.chromium.org/11725005/diff/6008/ui/base/x/x11_util.cc File ui/base/x/x11_util.cc (right): https://codereview.chromium.org/11725005/diff/6008/ui/base/x/x11_util.cc#newc... ui/base/x/x11_util.cc:1392: bool GetOutputOverscanFlag(XID output) { this doesn't match the name from the header, does it? https://codereview.chromium.org/11725005/diff/6008/ui/base/x/x11_util.h File ui/base/x/x11_util.h (right): https://codereview.chromium.org/11725005/diff/6008/ui/base/x/x11_util.h#newco... ui/base/x/x11_util.h:279: // Returns true if the EDID for the output has the flag of overscan. Note that nit: s/the flag of overscan/its overscan flag set/ https://codereview.chromium.org/11725005/diff/6008/ui/base/x/x11_util.h#newco... ui/base/x/x11_util.h:280: // returning false does not mean no overscan, it just means the lack of flag. nit: s/no overscan/overscan is disabled/, s/lack of flag/flag wasn't present/ https://codereview.chromium.org/11725005/diff/6008/ui/base/x/x11_util.h#newco... ui/base/x/x11_util.h:281: UI_EXPORT bool HasOutputOverscanFlag(XID output); nit: sorry for conflicting with an earlier suggestion, but a name like OutputOverscanFlagIsSet() might be better, since this is really testing both that the has the flag and that the flag is set -- Has...Flag() seems like it could also return true if the flag was there but was set to false. https://codereview.chromium.org/11725005/diff/6008/ui/base/x/x11_util.h#newco... ui/base/x/x11_util.h:285: // for x11_util_unittest.cc. nit: document that the last three arguments can be NULL if the caller doesn't care about the values
https://codereview.chromium.org/11725005/diff/6008/ui/base/x/x11_util.h File ui/base/x/x11_util.h (right): https://codereview.chromium.org/11725005/diff/6008/ui/base/x/x11_util.h#newco... ui/base/x/x11_util.h:279: // Returns true if the EDID for the output has the flag of overscan. Note that On 2013/01/04 00:19:12, Daniel Erat wrote: > nit: s/the flag of overscan/its overscan flag set/ Done. https://codereview.chromium.org/11725005/diff/6008/ui/base/x/x11_util.h#newco... ui/base/x/x11_util.h:280: // returning false does not mean no overscan, it just means the lack of flag. On 2013/01/04 00:19:12, Daniel Erat wrote: > nit: s/no overscan/overscan is disabled/, s/lack of flag/flag wasn't present/ Hmm, 'overscan is disabled' sounds like a bit weird to me. I rewrote the whole comment, so please take another look. Sorry! https://codereview.chromium.org/11725005/diff/6008/ui/base/x/x11_util.h#newco... ui/base/x/x11_util.h:281: UI_EXPORT bool HasOutputOverscanFlag(XID output); On 2013/01/04 00:19:12, Daniel Erat wrote: > nit: sorry for conflicting with an earlier suggestion, but a name like > OutputOverscanFlagIsSet() might be better, since this is really testing both > that the has the flag and that the flag is set -- Has...Flag() seems like it > could also return true if the flag was there but was set to false. I also changed my mind and it would be good to distinguish the existence of the flag and the contents of the flag. So it's consistent with GetOutputDeviceData(), and renamed it to bool GetOutputOverscanFlag(XID, bool*); https://codereview.chromium.org/11725005/diff/6008/ui/base/x/x11_util.h#newco... ui/base/x/x11_util.h:285: // for x11_util_unittest.cc. On 2013/01/04 00:19:12, Daniel Erat wrote: > nit: document that the last three arguments can be NULL if the caller doesn't > care about the values Done.
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mukai@chromium.org/11725005/14002
Presubmit check for 11725005-14002 failed and returned exit status 1. Running presubmit commit checks ... ** Presubmit Messages ** You might be calling functions intended only for testing from production code. It is OK to ignore this warning if you know what you are doing, as the heuristics used to detect the situation are not perfect. The commit queue will not block on this warning. Email joi@chromium.org if you have questions. ui/base/x/x11_util.cc:1645 void InitXKeyEventForTesting(EventType type, ** Presubmit ERRORS ** Missing LGTM from an OWNER for files in these directories: ui Presubmit checks took 3.9s to calculate.
oops, I needed the owner of ui/ itself. Adding sky to reviewer and wait for his back.
Rubber stamp LGTM
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mukai@chromium.org/11725005/14002
Retried try job too often on win_aura for step(s) content_browsertests, interactive_ui_tests
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mukai@chromium.org/11725005/14002
Retried try job too often on win_aura for step(s) interactive_ui_tests |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
