Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
689083002:
Right clicking outside of a selected area should deselect text and bring up the correct context men… (Closed)
DescriptionRight clicking outside of a selected area should deselect text and bring up the correct context menu.
on right clink selection was not getting cleared,due to that
wrong context menu is coming.
Changes done so that when right click on the selected area then
selection will stay and proper context menu will come.
And when right click have been done outside selection then
selection will clear and correct context menu will come.
BUG=92800
Committed: https://crrev.com/8f17ec1cb198cd940d10a9c499d87c118aecd39d
Cr-Commit-Position: refs/heads/master@{#310706}
Patch Set 1 #Patch Set 2 : addressing nit. #Patch Set 3 : Code refactor improvement. #Patch Set 4 : Refactoring and Improving patch. #
Total comments: 10
Patch Set 5 : Changes as per review comments. #
Total comments: 8
Patch Set 6 : Changes as per review comments #
Total comments: 2
Patch Set 7 : Addressing nit. #Messages
Total messages: 27 (4 generated)
deepak.m1@samsung.com changed reviewers: + raymes@chromium.org
PTAL.
On 2014/10/31 12:01:32, Deepak wrote: > PTAL. Hi @raymes, Please review my changes. Thanks
On 2014/11/03 13:50:32, Deepak wrote: > On 2014/10/31 12:01:32, Deepak wrote: > > PTAL. Hi @raymes, Please review my changes. Thanks
On 2014/11/05 11:53:58, Deepak wrote: > On 2014/11/03 13:50:32, Deepak wrote: > > On 2014/10/31 12:01:32, Deepak wrote: > > > PTAL. Hi @raymes, Please review my changes. Thanks
On 2014/11/07 11:20:45, Deepak wrote: > On 2014/11/05 11:53:58, Deepak wrote: > > On 2014/11/03 13:50:32, Deepak wrote: > > > On 2014/10/31 12:01:32, Deepak wrote: > > > > PTAL. > @raymes, PTAL at my changes.
On 2014/11/14 06:12:45, Deepak wrote: > On 2014/11/07 11:20:45, Deepak wrote: > > On 2014/11/05 11:53:58, Deepak wrote: > > > On 2014/11/03 13:50:32, Deepak wrote: > > > > On 2014/10/31 12:01:32, Deepak wrote: > > > > > PTAL. > > @raymes, PTAL at my changes.
Sorry again for the slow review on this and on https://codereview.chromium.org/697933002/. We've had some recent bugs in changes to pdfium_engine.cc and it's very untested code. Because of this I wanted to review it really closely before accepting but I haven't had time. Is it possible we can add a test for this (and the other patch). Starting to add tests or refactor pieces of this file would help a lot with its code health. Thanks!
On 2014/11/22 07:29:15, raymes (OOO 11-27 to 12-14) wrote: > Sorry again for the slow review on this and on > https://codereview.chromium.org/697933002/. We've had some recent bugs in > changes to pdfium_engine.cc and it's very untested code. Because of this I > wanted to review it really closely before accepting but I haven't had time. > > Is it possible we can add a test for this (and the other patch). Starting to add > tests or refactor pieces of this file would help a lot with its code health. > > Thanks! Thanks raymes for review. I have refactored my earlier patch to separate out common code and tested more than a hours again thoroughly to check any side effect if any. PTAL whenever you got time. Thanks
PTAL.
PTAL.
PTAL.
deepak.m1@samsung.com changed reviewers: + gene@chromium.org
@Raymes and @Gene PTAL.
@Raymes and @Gene PTAL at my changes. Thanks
gene@chromium.org changed reviewers: + jam@chromium.org
@Gene PTAL at my changes. Thanks
https://codereview.chromium.org/689083002/diff/60001/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/689083002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:1608: if (event.GetButton() != PP_INPUTEVENT_MOUSEBUTTON_LEFT) { We probably don't want to erase selection for middle button click. https://codereview.chromium.org/689083002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:1616: selection_rect = selection_rect.Union(selection_rect_vector[i]); Why do you want to unite all rects, instead of testing event.GetPosition() against each rect separately? https://codereview.chromium.org/689083002/diff/60001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/689083002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.h:249: void GetAllScreenRectsUnion(std::vector<PDFiumRange> rectRange, nit: variable names are not camel case (like offset_point below) nit2: please use const reference here instead of pass by value https://codereview.chromium.org/689083002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.h:250: const pp::Point offset_point, nit: const reference? https://codereview.chromium.org/689083002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.h:251: std::vector<pp::Rect>& rectVector); nit: variable names are not camel case (like offset_point above) nit2: if you need to return vector, please use pointer instead of reference (see CheckPageAvailable above)
@gene Review comments addressed. PTAL. Thanks https://codereview.chromium.org/689083002/diff/60001/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/689083002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:1608: if (event.GetButton() != PP_INPUTEVENT_MOUSEBUTTON_LEFT) { On 2015/01/06 18:40:04, gene wrote: > We probably don't want to erase selection for middle button click. Done. https://codereview.chromium.org/689083002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:1616: selection_rect = selection_rect.Union(selection_rect_vector[i]); On 2015/01/06 18:40:04, gene wrote: > Why do you want to unite all rects, instead of testing event.GetPosition() > against each rect separately? I agree with you. This will help us in early return. https://codereview.chromium.org/689083002/diff/60001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/689083002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.h:249: void GetAllScreenRectsUnion(std::vector<PDFiumRange> rectRange, On 2015/01/06 18:40:04, gene wrote: > nit: variable names are not camel case (like offset_point below) > nit2: please use const reference here instead of pass by value changed to reference, but can not use const as GetScreenRect() is not marked as const. https://codereview.chromium.org/689083002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.h:250: const pp::Point offset_point, On 2015/01/06 18:40:04, gene wrote: > nit: const reference? Done. https://codereview.chromium.org/689083002/diff/60001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.h:251: std::vector<pp::Rect>& rectVector); On 2015/01/06 18:40:04, gene wrote: > nit: variable names are not camel case (like offset_point above) > nit2: if you need to return vector, please use pointer instead of reference (see > CheckPageAvailable above) Done.
https://codereview.chromium.org/689083002/diff/80001/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/689083002/diff/80001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:1608: if (event.GetButton() == PP_INPUTEVENT_MOUSEBUTTON_MIDDLE) nit: could you please check (GetButton() != LEFT && GetButton() != right), it will be a bit more future proof if new ones are added/reported (e.g. PP_INPUTEVENT_MOUSEBUTTON_NONE sneaks in somehow?) https://codereview.chromium.org/689083002/diff/80001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:1618: if (selection_rect_vector[i].IsEmpty() || Could you please explain why you exit here if at least one empty screen rect is found? https://codereview.chromium.org/689083002/diff/80001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/689083002/diff/80001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.h:249: void GetAllScreenRectsUnion(std::vector<PDFiumRange>& rect_range, Could you please make it pointer then (since GetScreenRects() is not const)? non-const references are against Chrome style guide: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arg... https://codereview.chromium.org/689083002/diff/80001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.h:250: const pp::Point offset_point, nit: const reference please
@gene Thanks for review. I have done changes as suggested. PTAL. https://codereview.chromium.org/689083002/diff/80001/pdf/pdfium/pdfium_engine.cc File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/689083002/diff/80001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:1608: if (event.GetButton() == PP_INPUTEVENT_MOUSEBUTTON_MIDDLE) On 2015/01/07 23:16:01, gene wrote: > nit: could you please check (GetButton() != LEFT && GetButton() != right), it > will be a bit more future proof if new ones are added/reported (e.g. > PP_INPUTEVENT_MOUSEBUTTON_NONE sneaks in somehow?) Done. https://codereview.chromium.org/689083002/diff/80001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.cc:1618: if (selection_rect_vector[i].IsEmpty() || On 2015/01/07 23:16:01, gene wrote: > Could you please explain why you exit here if at least one empty screen rect is > found? first condition is not required so removing this. https://codereview.chromium.org/689083002/diff/80001/pdf/pdfium/pdfium_engine.h File pdf/pdfium/pdfium_engine.h (right): https://codereview.chromium.org/689083002/diff/80001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.h:249: void GetAllScreenRectsUnion(std::vector<PDFiumRange>& rect_range, On 2015/01/07 23:16:01, gene wrote: > Could you please make it pointer then (since GetScreenRects() is not const)? > non-const references are against Chrome style guide: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Arg... Done. https://codereview.chromium.org/689083002/diff/80001/pdf/pdfium/pdfium_engine... pdf/pdfium/pdfium_engine.h:250: const pp::Point offset_point, On 2015/01/07 23:16:01, gene wrote: > nit: const reference please Done.
LGTM with one nit https://codereview.chromium.org/689083002/diff/100001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/689083002/diff/100001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:2213: (*it).GetScreenRects(offset_point, current_zoom_, current_rotation_); nit: change "(*it)." to "it->"
Nit changes done. https://codereview.chromium.org/689083002/diff/100001/pdf/pdfium/pdfium_engin... File pdf/pdfium/pdfium_engine.cc (right): https://codereview.chromium.org/689083002/diff/100001/pdf/pdfium/pdfium_engin... pdf/pdfium/pdfium_engine.cc:2213: (*it).GetScreenRects(offset_point, current_zoom_, current_rotation_); On 2015/01/08 18:27:57, gene wrote: > nit: change "(*it)." to "it->" Done.
The CQ bit was checked by deepak.m1@samsung.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/689083002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/8f17ec1cb198cd940d10a9c499d87c118aecd39d Cr-Commit-Position: refs/heads/master@{#310706} |
