Start list rearrangement at requested item [pdfium : main]
Kuka (Gerrit)
Kuka has uploaded the change for review![]( "Open in Gerrit")
Commit message
Start list rearrangement at requested item
Limit CPWL_ListCtrl::ReArrange() to walk from the requested item index, and handle empty lists before clamping the start index.
BUG=477200528
Change diff
diff --git a/fpdfsdk/pwl/cpwl_list_ctrl.cpp b/fpdfsdk/pwl/cpwl_list_ctrl.cpp
index 6e9db91..58b3f7b 100644
--- a/fpdfsdk/pwl/cpwl_list_ctrl.cpp
+++ b/fpdfsdk/pwl/cpwl_list_ctrl.cpp
@@ -516,12 +516,22 @@
}
void CPWL_ListCtrl::ReArrange(int32_t nItemIndex) {
+ const int32_t nCount = GetCount();
+ if (nCount == 0) {
+ content_rect_ = CFX_FloatRect();
+ SetScrollInfo();
+ return;
+ }
+
+ nItemIndex = std::max(0, std::min(nItemIndex, nCount - 1));
+
float fPosY = 0.0f;
if (IsValid(nItemIndex - 1)) {
fPosY = list_items_[nItemIndex - 1]->GetRect().bottom;
}
- for (const auto& pListItem : list_items_) {
+ for (int32_t i = nItemIndex; i < nCount; ++i) {
+ const auto& pListItem = list_items_[i];
float fListItemHeight = pListItem->GetItemHeight();
pListItem->SetRect(
CFX_FloatRect(0.0f, fPosY + fListItemHeight, 0.0f, fPosY));
Change information
- M fpdfsdk/pwl/cpwl_list_ctrl.cpp
Related details
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 6 comments![]( "Open in Gerrit")
Limit CPWL_ListCtrl::ReArrange() to walk from the requested item index, and handle empty lists before clamping the start index.Wrap at 72 columns.
const int32_t nCount = GetCount();In general, PDFium is trying to use Google C++ style naming going forward. So this can be named "count".
if (nCount == 0) {Check `list_items_.empty()` instead.
nItemIndex = std::max(0, std::min(nItemIndex, nCount - 1));Given the callers, I believe this pre-condition should hold: `CHECK_GE(nItemIndex, 0);`
If this CL adds that to the top of this method, then the std::max() part here can be removed, right?
for (int32_t i = nItemIndex; i < nCount; ++i) {Instead of indexing here, how about something like `for (const auto& pListItem : pdfium::span(list_items_).subspan(nItemIndex)) {` ?
Related details
- Kuka
- Tom Sepez
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Tom Sepez added 2 comments![]( "Open in Gerrit")
void CPWL_ListCtrl::ReArrange(int32_t nItemIndex) {nit: pre-existing: can we call this Rearrange instead of ReArrange while we're at it?
.subspan(pdfium::checked_cast<size_t>(nItemIndex))) {Having checked GE above, we know that nItemIndex is valid as a size_t.
Related details
- Kuka
- Lei Zhang
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 2 comments![]( "Open in Gerrit")
void CPWL_ListCtrl::ReArrange(int32_t nItemIndex) {nit: pre-existing: can we call this Rearrange instead of ReArrange while we're at it?
I figured someone can do this as a follow-up, and convert `nItemIndex` to `size_t`.
.subspan(pdfium::checked_cast<size_t>(nItemIndex))) {Having checked GE above, we know that nItemIndex is valid as a size_t.
Or maybe something like: `size_t actual_index = std::min<size_t>(nItemIndex, list_items_.size() - 1);`
Either way, no need for `checked_cast()` here.
Related details
- Kuka
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
Run this through optipng to shrink the PNG a bit.
BTW, this will likely require different expectation files for Win/Mac/Linux.
Related details
- Kuka
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
Run this through optipng to shrink the PNG a bit.
BTW, this will likely require different expectation files for Win/Mac/Linux.
https://ci.chromium.org/ui/p/pdfium/builders/try/mac_no_v8/32476/test-results should show the comparison between this file and what the Mac try bot rendered. Add that to the CL? I ran try jobs on all 3 platforms with AGG and Skia.
Related details
- Kuka
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kuka (Gerrit)
Kuka added 7 comments![]( "Open in Gerrit")
Limit CPWL_ListCtrl::ReArrange() to walk from the requested item index, and handle empty lists before clamping the start index.KukaWrap at 72 columns.
Done
In general, PDFium is trying to use Google C++ style naming going forward. So this can be named "count".
Done
if (nCount == 0) {KukaCheck `list_items_.empty()` instead.
Done
nItemIndex = std::max(0, std::min(nItemIndex, nCount - 1));Given the callers, I believe this pre-condition should hold: `CHECK_GE(nItemIndex, 0);`
If this CL adds that to the top of this method, then the std::max() part here can be removed, right?
Done
Instead of indexing here, how about something like `for (const auto& pListItem : pdfium::span(list_items_).subspan(nItemIndex)) {` ?
Done
Lei ZhangRun this through optipng to shrink the PNG a bit.
BTW, this will likely require different expectation files for Win/Mac/Linux.
https://ci.chromium.org/ui/p/pdfium/builders/try/mac_no_v8/32476/test-results should show the comparison between this file and what the Mac try bot rendered. Add that to the CL? I ran try jobs on all 3 platforms with AGG and Skia.
Thanks. Just to confirm, do I only need to add the Mac expectation image from the try bot results, or should I add expectation images for all platform/backend combinations that differ (Win/Linux/Mac, AGG/Skia)?
Related details
- Lei Zhang
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kuka (Gerrit)
Kuka added 2 comments![]( "Open in Gerrit")
Lei Zhangnit: pre-existing: can we call this Rearrange instead of ReArrange while we're at it?
I figured someone can do this as a follow-up, and convert `nItemIndex` to `size_t`.
Done
Lei ZhangHaving checked GE above, we know that nItemIndex is valid as a size_t.
Or maybe something like: `size_t actual_index = std::min<size_t>(nItemIndex, list_items_.size() - 1);`
Either way, no need for `checked_cast()` here.
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
Lei ZhangRun this through optipng to shrink the PNG a bit.
BTW, this will likely require different expectation files for Win/Mac/Linux.
Kukahttps://ci.chromium.org/ui/p/pdfium/builders/try/mac_no_v8/32476/test-results should show the comparison between this file and what the Mac try bot rendered. Add that to the CL? I ran try jobs on all 3 platforms with AGG and Skia.
Thanks. Just to confirm, do I only need to add the Mac expectation image from the try bot results, or should I add expectation images for all platform/backend combinations that differ (Win/Linux/Mac, AGG/Skia)?
The CL needs to have the variations to make the try bots happy across Win/Mac/Linux. See the failures here: https://pdfium-review.googlesource.com/c/pdfium/+/148610?checksPatchset=4&tab=checks
Related details
- Kuka
- Tom Sepez
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kuka (Gerrit)
Kuka added 2 comments![]( "Open in Gerrit")
.subspan(pdfium::checked_cast<size_t>(nItemIndex))) {Lei ZhangHaving checked GE above, we know that nItemIndex is valid as a size_t.
KukaOr maybe something like: `size_t actual_index = std::min<size_t>(nItemIndex, list_items_.size() - 1);`
Either way, no need for `checked_cast()` here.
Done
The build fails at fpdfsdk/pwl/cpwl_list_ctrl.cpp:537:
for (const auto& pListItem : pdfium::span(list_items_).subspan(nItemIndex)) {`nItemIndex` is `int32_t`, but `subspan()` wraps its argument in
`StrictNumeric<size_t>`. The constructor of `StrictNumeric` calls
`strict_cast<size_t>(int32_t_value)`, which is a compile-time error
because `unsigned long long` cannot represent all `int` values
(i.e., negative numbers fail the `kContained` range check).
Even though `CHECK_GE(nItemIndex, 0)` at line 521 guarantees
`nItemIndex` is non-negative at runtime, the compiler doesn't see that.
The fix should be either:
.subspan(pdfium::checked_cast<size_t>(nItemIndex))
or computing a `size_t` index upfront:
size_t index = std::min<size_t>(nItemIndex, list_items_.size() - 1);
.subspan(pdfium::checked_cast<size_t>(nItemIndex))) {Lei ZhangHaving checked GE above, we know that nItemIndex is valid as a size_t.
KukaOr maybe something like: `size_t actual_index = std::min<size_t>(nItemIndex, list_items_.size() - 1);`
Either way, no need for `checked_cast()` here.
Done
Build error at cpwl_list_ctrl.cpp:537:
.subspan(nItemIndex)
`nItemIndex` is `int32_t`, but `subspan()` expects a `size_t`.
The argument is wrapped in `StrictNumeric<size_t>`, which performs a
`strict_cast<size_t>()`. This fails to compile because `size_t` cannot
represent all possible `int32_t` values.
Although `CHECK_GE(nItemIndex, 0)` guarantees the value is non-negative
at runtime, the compiler cannot infer that. Use:
.subspan(pdfium::checked_cast<size_t>(nItemIndex))
Related details
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
.subspan(pdfium::checked_cast<size_t>(nItemIndex))) {Lei ZhangHaving checked GE above, we know that nItemIndex is valid as a size_t.
KukaOr maybe something like: `size_t actual_index = std::min<size_t>(nItemIndex, list_items_.size() - 1);`
Either way, no need for `checked_cast()` here.
KukaDone
Build error at cpwl_list_ctrl.cpp:537:
.subspan(nItemIndex)`nItemIndex` is `int32_t`, but `subspan()` expects a `size_t`.
The argument is wrapped in `StrictNumeric<size_t>`, which performs a
`strict_cast<size_t>()`. This fails to compile because `size_t` cannot
represent all possible `int32_t` values.Although `CHECK_GE(nItemIndex, 0)` guarantees the value is non-negative
at runtime, the compiler cannot infer that. Use:.subspan(pdfium::checked_cast<size_t>(nItemIndex))
After the `CHECK_GE()` assertion, it should be safe to use `static_cast<size_t>()`. You can certainly also calculate the index as a separate size_t variable.
Related details
- Kuka
- Tom Sepez
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kuka (Gerrit)
Kuka added 2 comments![]( "Open in Gerrit")
Uploaded the expected images for the other platform/backend variants, and kept
the code change limited to the subspan() index conversion. Could you please run
CQ again to verify the updated expectations?
.subspan(pdfium::checked_cast<size_t>(nItemIndex))) {Lei ZhangHaving checked GE above, we know that nItemIndex is valid as a size_t.
KukaOr maybe something like: `size_t actual_index = std::min<size_t>(nItemIndex, list_items_.size() - 1);`
Either way, no need for `checked_cast()` here.
KukaDone
Lei ZhangBuild error at cpwl_list_ctrl.cpp:537:
.subspan(nItemIndex)`nItemIndex` is `int32_t`, but `subspan()` expects a `size_t`.
The argument is wrapped in `StrictNumeric<size_t>`, which performs a
`strict_cast<size_t>()`. This fails to compile because `size_t` cannot
represent all possible `int32_t` values.Although `CHECK_GE(nItemIndex, 0)` guarantees the value is non-negative
at runtime, the compiler cannot infer that. Use:.subspan(pdfium::checked_cast<size_t>(nItemIndex))
After the `CHECK_GE()` assertion, it should be safe to use `static_cast<size_t>()`. You can certainly also calculate the index as a separate size_t variable.
I kept the existing nItemIndex logic unchanged and only changed
the subspan() argument to static_cast<size_t>(nItemIndex), as suggested.
Related details
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Tom Sepez voted Code-Review+1![]( "Open in Gerrit")
- Kuka
- Lei Zhang
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang voted and added 2 comments![]( "Open in Gerrit")
Votes added by Lei Zhang
| Code-Review | +1 |
| Commit-Queue | +1 |
2 comments
Uploaded the expected images for the other platform/backend variants, and kept
the code change limited to the subspan() index conversion. Could you please run
CQ again to verify the updated expectations?
Acknowledged
Lei ZhangRun this through optipng to shrink the PNG a bit.
BTW, this will likely require different expectation files for Win/Mac/Linux.
Kukahttps://ci.chromium.org/ui/p/pdfium/builders/try/mac_no_v8/32476/test-results should show the comparison between this file and what the Mac try bot rendered. Add that to the CL? I ran try jobs on all 3 platforms with AGG and Skia.
Lei ZhangThanks. Just to confirm, do I only need to add the Mac expectation image from the try bot results, or should I add expectation images for all platform/backend combinations that differ (Win/Linux/Mac, AGG/Skia)?
The CL needs to have the variations to make the try bots happy across Win/Mac/Linux. See the failures here: https://pdfium-review.googlesource.com/c/pdfium/+/148610?checksPatchset=4&tab=checks
Acknowledged
Related details
- Kuka
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
Needs an entry for this test case in testing/SUPPRESSIONS around line 370.
Related details
- Kuka
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kuka (Gerrit)
Kuka added 1 comment![]( "Open in Gerrit")
I updated testing/SUPPRESSIONS. Would you mind running CQ again when you have a
chance? Thank you!
Related details
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang voted![]( "Open in Gerrit")
- Kuka
- Tom Sepez
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
I updated testing/SUPPRESSIONS. Would you mind running CQ again when you have a
chance? Thank you!
Done
Related details
- Kuka
- Tom Sepez
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tom Sepez (Gerrit)
Tom Sepez voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Kuka
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
bug_477200528.pdf * * * gdiActually needs to be near line 767 in the other list below. List the file as bug_477200528.in. My earlier comment was bad advice.
Related details
- Kuka
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kuka (Gerrit)
Kuka added 1 comment![]( "Open in Gerrit")
bug_477200528.pdf * * * gdiActually needs to be near line 767 in the other list below. List the file as bug_477200528.in. My earlier comment was bad advice.
I uploaded the fix for testing/SUPPRESSIONS. It now lists bug_477200528.in in
the GDI suppression list as suggested. Could you please take another look when
you have a chance?
Related details
- Lei Zhang
- Tom Sepez
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang voted and added 1 comment![]( "Open in Gerrit")
Votes added by Lei Zhang
| Code-Review | +1 |
| Commit-Queue | +1 |
1 comment
bug_477200528.pdf * * * gdiKukaActually needs to be near line 767 in the other list below. List the file as bug_477200528.in. My earlier comment was bad advice.
I uploaded the fix for testing/SUPPRESSIONS. It now lists bug_477200528.in in
the GDI suppression list as suggested. Could you please take another look when
you have a chance?
- Kuka
- Tom Sepez
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |