For loop is running un-nacessrily after match.

For loop is running un-nacessrily after match.

loop should break after match as values are getting reset to empty
rect after match.Changes done so that loop breaks after match.

BUG=417621
Committed: https://crrev.com/0c4d9f76e8e10ac411f3c1ffcf7beaafc12746d2
Cr-Commit-Position: refs/heads/master@{#297138}

[Deepak, 2014-09-25 09:41:18]

deepak.m1@samsung.com changed reviewers:
+ raymes@chromium.org

[Deepak, 2014-09-25 09:41:18]

PTAL

[Deepak, 2014-09-25 09:42:26]

deepak.m1@samsung.com changed reviewers:
+ jam@chromium.org

[Deepak, 2014-09-25 09:42:26]

PTAL

[Deepak, 2014-09-26 03:23:51]

deepak.m1@samsung.com changed reviewers:
+ gene@chromium.org

[raymes, 2014-09-29 01:09:52]

I don't think this optimization gives correct results.

https://codereview.chromium.org/603903002/diff/1/pdf/pdfium/pdfium_engine.cc
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/603903002/diff/1/pdf/pdfium/pdfium_engine.cc#...
pdf/pdfium/pdfium_engine.cc:2729: break;
I don't think we can early return from this loop (though I could be wrong). To
me it appears that this loop is trying to find which selections haven't changed
so that we don't need to invalidate those regions on the screen. AFAICT this
requires searching through all of them.

[Deepak, 2014-09-29 02:41:16]

On 2014/09/29 01:09:52, raymes wrote:
> I don't think this optimization gives correct results.
> 
> https://codereview.chromium.org/603903002/diff/1/pdf/pdfium/pdfium_engine.cc
> File pdf/pdfium/pdfium_engine.cc (right):
> 
>
https://codereview.chromium.org/603903002/diff/1/pdf/pdfium/pdfium_engine.cc#...
> pdf/pdfium/pdfium_engine.cc:2729: break;
> I don't think we can early return from this loop (though I could be wrong). To
> me it appears that this loop is trying to find which selections haven't
changed
> so that we don't need to invalidate those regions on the screen. AFAICT this
> requires searching through all of them.

Thanks raymes, for review.
Pleaes have a look at my comments.

[Deepak, 2014-09-29 02:41:28]

PTAL

https://codereview.chromium.org/603903002/diff/1/pdf/pdfium/pdfium_engine.cc
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/603903002/diff/1/pdf/pdfium/pdfium_engine.cc#...
pdf/pdfium/pdfium_engine.cc:2729: break;
On 2014/09/29 01:09:51, raymes wrote:
> I don't think we can early return from this loop (though I could be wrong). To
> me it appears that this loop is trying to find which selections haven't
changed
> so that we don't need to invalidate those regions on the screen. AFAICT this
> requires searching through all of them.

@raymes, When below condition 
new_selections[i] == old_selections_[j]
matchs , Then we are setting both new_selection[i] and old+selection[j] as empty
rect, After that if we run inner loop then 
if old_selection is empty rect then only condition will match as new_selection
is already set empty rect.
and what is the point of checking empty rect's match in condition and then
assigning them empty rect again?

As their is no point of checking condition if old_selection is empty.
so this changes will avoid un-neccessary loop running.

[raymes, 2014-09-29 03:08:16]

https://codereview.chromium.org/603903002/diff/1/pdf/pdfium/pdfium_engine.cc
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/603903002/diff/1/pdf/pdfium/pdfium_engine.cc#...
pdf/pdfium/pdfium_engine.cc:2729: break;
Oh I see! Is it ever possible for old_selections_ to contain duplicates? I guess
not but I haven't checked.

[Deepak, 2014-09-29 03:19:52]

PTAL

https://codereview.chromium.org/603903002/diff/1/pdf/pdfium/pdfium_engine.cc
File pdf/pdfium/pdfium_engine.cc (right):

https://codereview.chromium.org/603903002/diff/1/pdf/pdfium/pdfium_engine.cc#...
pdf/pdfium/pdfium_engine.cc:2729: break;
On 2014/09/29 03:08:16, raymes wrote:
> Oh I see! Is it ever possible for old_selections_ to contain duplicates? I
guess
> not but I haven't checked.

I think old selection does not contain duplicates.
And even it contains duplicates it will naver match as we have alreay set
new_selection[i] as empty rect :).

Little example:

new_selection: [A,C,D]
old_selection: [A,C,Z,A]

so in the above loop when new_selection[0] matches with old_selection[0]
then it is like
new_selection: [" ",C,D]
old_selection: [" ",C,Z,A]
then even old_selection have duplicate or not , new_seletion[0] will naver match
to any of the item in old_selction.So no need to run the loop.

Now when outerloop runs again then their is no need to match new_selection[1]
with old_selection[0] as old_selection[0] is already empty, then again
new_selection[1] will matchs with old_selection[1] and it will be 
new_selection: [" "," ",D]
old_selection: [" "," ",Z,A]
Then no point of running inner loop as new_selection[1] is empty and it will
only matchs with old_selection that have empty, and even after match their is no
point of setting them empty again as they are already empty.

[raymes, 2014-09-29 03:21:43]

On 2014/09/29 03:19:52, Deepak wrote:
> PTAL
> 
> https://codereview.chromium.org/603903002/diff/1/pdf/pdfium/pdfium_engine.cc
> File pdf/pdfium/pdfium_engine.cc (right):
> 
>
https://codereview.chromium.org/603903002/diff/1/pdf/pdfium/pdfium_engine.cc#...
> pdf/pdfium/pdfium_engine.cc:2729: break;
> On 2014/09/29 03:08:16, raymes wrote:
> > Oh I see! Is it ever possible for old_selections_ to contain duplicates? I
> guess
> > not but I haven't checked.
> 
> I think old selection does not contain duplicates.
> And even it contains duplicates it will naver match as we have alreay set
> new_selection[i] as empty rect :).
> 
> Little example:
> 
> new_selection: [A,C,D]
> old_selection: [A,C,Z,A]
> 
> so in the above loop when new_selection[0] matches with old_selection[0]
> then it is like
> new_selection: [" ",C,D]
> old_selection: [" ",C,Z,A]
> then even old_selection have duplicate or not , new_seletion[0] will naver
match
> to any of the item in old_selction.So no need to run the loop.
> 
> Now when outerloop runs again then their is no need to match new_selection[1]
> with old_selection[0] as old_selection[0] is already empty, then again
> new_selection[1] will matchs with old_selection[1] and it will be 
> new_selection: [" "," ",D]
> old_selection: [" "," ",Z,A]
> Then no point of running inner loop as new_selection[1] is empty and it will
> only matchs with old_selection that have empty, and even after match their is
no
> point of setting them empty again as they are already empty.

Good point, thanks for the explanation! lgtm

[Deepak, 2014-09-29 04:03:22]

The CQ bit was checked by deepak.m1@samsung.com

[commit-bot: I haz the power, 2014-09-29 04:03:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/603903002/1

[commit-bot: I haz the power, 2014-09-29 04:38:48]

Committed patchset #1 (id:1) as fd61c7a0c2bccf12067eb60bcf41d585752f6c17

[commit-bot: I haz the power, 2014-09-29 04:39:36]

Patchset 1 (id:??) landed as
https://crrev.com/0c4d9f76e8e10ac411f3c1ffcf7beaafc12746d2
Cr-Commit-Position: refs/heads/master@{#297138}