Issue metadata
Sign in to add a comment
|
Not able to load External pdf files |
||||||||||||||||||||||
Issue descriptionApp Version: 72.0.3603.0 Canary iOS Version: 10.3.3, 11.4.1, 12.0.1, 12.1 beta Device: iPhone and iPad Steps to reproduce: 1. Launch chrome app 2. Load sample pdf file in safari browser (http://www.pdf995.com/samples/pdf.pdf) 3. Tap on share icon and select copy to chrome canary Observed results: Notice that site can’t be reached error page is displayed Expected results: External pdf file should load Revision Number for 72.0.3591.0 (Bad Version) - 148434e1b31c Revision Number for 72.0.3585.0 (Good Version) - 2eea43d10d34 Number of times you were able to reproduce: 5/5 Bug reproducible after clean install: Yes Bug reproducible after clearing cache and cookies: Yes Bug reproducible on Chrome Mobile on Android: Not tested Bug reproducible on Safari/Firefox: Firefox: NA, Safari: NA Bug reproducible on current stable build (App Version, iOS Version): N0 on M70 Bug reproducible on the current beta channel build (App Version, iOS Version): No on M71 Beta Type-bug-regression? Yes Link to Video: https://drive.google.com/file/d/1aOjnNV-adkp937DpQAXrWirUKX1wjwOg/view?usp=sharing
,
Nov 8
,
Nov 9
,
Dec 10
I dont think i'm the right owner of that bug, assigning to eugene who may know who is
,
Dec 10
I believe this CL is a culprit: https://chromium-review.googlesource.com/c/1255611 For some reason hasControllerForURL: returns NO for kChromeUIExternalFileHost. Which seems to be a bug.
,
Dec 10
,
Dec 11
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/630f1690aac12a8a96b9e4edccae8718834c3a68 commit 630f1690aac12a8a96b9e4edccae8718834c3a68 Author: Eugene But <eugenebut@google.com> Date: Tue Dec 11 02:11:07 2018 Fix external file presentation. This bug is a regression, because ios/web started presenting error pages if hasControllerForURL: returns NO and the URL is not a WebUI URL. ios/web change was a reasonable change, but BVC did not follow API contract and returned NO from hasControllerForURL: even if controller existed. Return YES from hasControllerForURL: for kChromeUIExternalFileHost if NativeController actually exists. This change enforces existing API constract for CRWNativeContentProvider. EG tests do not have good infrastructure to write end-to-end test for external URLs. BVC is also not testable, so this CL does not include tests. Bug: 902271 Change-Id: I81c08b439460bcbf81f0e7234f9cdbe57938e6fe Reviewed-on: https://chromium-review.googlesource.com/c/1370931 Commit-Queue: Eugene But <eugenebut@chromium.org> Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org> Cr-Commit-Position: refs/heads/master@{#615384} [modify] https://crrev.com/630f1690aac12a8a96b9e4edccae8718834c3a68/ios/chrome/browser/ui/browser_view_controller.mm
,
Dec 11
crrev.com/c/1370931 adds native content back, but the UI looks broken for iPhone X, because nativeContentHeaderHeightForWebState: returns incorrect value. I think this is a regression, but it's hard to say when it happened because PDF was not rendered for a while. Kurt, would you mind looking why nativeContentHeaderHeightForWebState: returns incorrect value?
,
Dec 14
,
Dec 19
Issue 913428 has been merged into this issue.
,
Dec 22
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a36505e85442fa4224253dad25a05f979611ef44 commit a36505e85442fa4224253dad25a05f979611ef44 Author: Kurt Horimoto <kkhorimoto@chromium.org> Date: Sat Dec 22 04:03:10 2018 [iOS] Fix native header height calculations. This CL fixes BVC's ToolbarHeightProviderForFullscreen to be accurate for iPad NTPs, where the browser toolbars are visible. NTPs on iPhone or on compact widths still have the same behavior where the browser toolbars are hidden (height of 0.0) and the NTP manages its own internal toolbars. This CL uses the corrected viewport insets in order to calculate the visible portions of the content area in order to lay out the NTP and specify the inset to use for native controllers. The referenced bug occurred because BVC's old implementation of |-nativeContentHeaderHeightForWebState:| returned the header height, which caused the native content to be mistakenly pushed down despite the fact that WebState::GetView() was already laid out below the toolbars on iOS 11. Bug: 902271 Change-Id: I364773e74b3d9d0ec7b433de5462666f21b1de5b Reviewed-on: https://chromium-review.googlesource.com/c/1378847 Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org> Reviewed-by: Eugene But <eugenebut@chromium.org> Reviewed-by: Gauthier Ambard <gambard@chromium.org> Cr-Commit-Position: refs/heads/master@{#618727} [modify] https://crrev.com/a36505e85442fa4224253dad25a05f979611ef44/ios/chrome/browser/prerender/preload_controller.mm [modify] https://crrev.com/a36505e85442fa4224253dad25a05f979611ef44/ios/chrome/browser/prerender/preload_controller_delegate.h [modify] https://crrev.com/a36505e85442fa4224253dad25a05f979611ef44/ios/chrome/browser/ui/browser_view_controller.mm [modify] https://crrev.com/a36505e85442fa4224253dad25a05f979611ef44/ios/web/public/test/fakes/test_native_content_provider.mm [modify] https://crrev.com/a36505e85442fa4224253dad25a05f979611ef44/ios/web/public/web_state/ui/crw_native_content_provider.h [modify] https://crrev.com/a36505e85442fa4224253dad25a05f979611ef44/ios/web/web_state/ui/crw_web_controller.mm [modify] https://crrev.com/a36505e85442fa4224253dad25a05f979611ef44/ios/web/web_state/ui/crw_web_controller_container_view.h [modify] https://crrev.com/a36505e85442fa4224253dad25a05f979611ef44/ios/web/web_state/ui/crw_web_controller_container_view.mm
,
Dec 22
,
Dec 22
This bug requires manual review: M72 has already been promoted to the beta branch, so this requires manual review Please contact the milestone owner if you have questions. Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 2
Note to TPM: there are 2 CLs which we want to merge.
,
Jan 3
eugenebut and kkhorimoto: canary verification please.
,
Jan 3
Vinutha, could you please verify this bug. Everyone else seems to be out. Thanks!
,
Jan 3
Verified on iOS 11.4.1 iPad Pro 12'9, iPhone XSMAX iOS 12.0.1 on 73.0.3660.0 Canary External pdf file loads in chrome .
,
Jan 4
Approved.
,
Jan 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/65a597771e3d8937da50aadf2ba98dc02ab72015 commit 65a597771e3d8937da50aadf2ba98dc02ab72015 Author: Eugene But <eugenebut@google.com> Date: Fri Jan 04 21:18:55 2019 Fix external file presentation. This bug is a regression, because ios/web started presenting error pages if hasControllerForURL: returns NO and the URL is not a WebUI URL. ios/web change was a reasonable change, but BVC did not follow API contract and returned NO from hasControllerForURL: even if controller existed. Return YES from hasControllerForURL: for kChromeUIExternalFileHost if NativeController actually exists. This change enforces existing API constract for CRWNativeContentProvider. EG tests do not have good infrastructure to write end-to-end test for external URLs. BVC is also not testable, so this CL does not include tests. TBR=eugenebut@google.com (cherry picked from commit 630f1690aac12a8a96b9e4edccae8718834c3a68) Bug: 902271 Change-Id: I81c08b439460bcbf81f0e7234f9cdbe57938e6fe Reviewed-on: https://chromium-review.googlesource.com/c/1370931 Commit-Queue: Eugene But <eugenebut@chromium.org> Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#615384} Reviewed-on: https://chromium-review.googlesource.com/c/1396607 Reviewed-by: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#567} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/65a597771e3d8937da50aadf2ba98dc02ab72015/ios/chrome/browser/ui/browser_view_controller.mm
,
Jan 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/088b5a224a03b25700959dcf9a97ad9ddc46182b Commit: 088b5a224a03b25700959dcf9a97ad9ddc46182b Author: eugenebut@google.com Commiter: eugenebut@chromium.org Date: 2019-01-04 21:20:22 +0000 UTC [iOS] Fix native header height calculations. This CL fixes BVC's ToolbarHeightProviderForFullscreen to be accurate for iPad NTPs, where the browser toolbars are visible. NTPs on iPhone or on compact widths still have the same behavior where the browser toolbars are hidden (height of 0.0) and the NTP manages its own internal toolbars. This CL uses the corrected viewport insets in order to calculate the visible portions of the content area in order to lay out the NTP and specify the inset to use for native controllers. The referenced bug occurred because BVC's old implementation of |-nativeContentHeaderHeightForWebState:| returned the header height, which caused the native content to be mistakenly pushed down despite the fact that WebState::GetView() was already laid out below the toolbars on iOS 11. TBR=kkhorimoto@chromium.org (cherry picked from commit a36505e85442fa4224253dad25a05f979611ef44) Bug: 902271 Change-Id: I364773e74b3d9d0ec7b433de5462666f21b1de5b Reviewed-on: https://chromium-review.googlesource.com/c/1378847 Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org> Reviewed-by: Eugene But <eugenebut@chromium.org> Reviewed-by: Gauthier Ambard <gambard@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#618727} Reviewed-on: https://chromium-review.googlesource.com/c/1395810 Cr-Commit-Position: refs/branch-heads/3626@{#568} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
,
Jan 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/65a597771e3d8937da50aadf2ba98dc02ab72015 Commit: 65a597771e3d8937da50aadf2ba98dc02ab72015 Author: eugenebut@google.com Commiter: eugenebut@chromium.org Date: 2019-01-04 21:18:55 +0000 UTC Fix external file presentation. This bug is a regression, because ios/web started presenting error pages if hasControllerForURL: returns NO and the URL is not a WebUI URL. ios/web change was a reasonable change, but BVC did not follow API contract and returned NO from hasControllerForURL: even if controller existed. Return YES from hasControllerForURL: for kChromeUIExternalFileHost if NativeController actually exists. This change enforces existing API constract for CRWNativeContentProvider. EG tests do not have good infrastructure to write end-to-end test for external URLs. BVC is also not testable, so this CL does not include tests. TBR=eugenebut@google.com (cherry picked from commit 630f1690aac12a8a96b9e4edccae8718834c3a68) Bug: 902271 Change-Id: I81c08b439460bcbf81f0e7234f9cdbe57938e6fe Reviewed-on: https://chromium-review.googlesource.com/c/1370931 Commit-Queue: Eugene But <eugenebut@chromium.org> Reviewed-by: Kurt Horimoto <kkhorimoto@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#615384} Reviewed-on: https://chromium-review.googlesource.com/c/1396607 Reviewed-by: Eugene But <eugenebut@chromium.org> Cr-Commit-Position: refs/branch-heads/3626@{#567} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
,
Jan 4
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/088b5a224a03b25700959dcf9a97ad9ddc46182b commit 088b5a224a03b25700959dcf9a97ad9ddc46182b Author: Eugene But <eugenebut@google.com> Date: Fri Jan 04 21:20:22 2019 [iOS] Fix native header height calculations. This CL fixes BVC's ToolbarHeightProviderForFullscreen to be accurate for iPad NTPs, where the browser toolbars are visible. NTPs on iPhone or on compact widths still have the same behavior where the browser toolbars are hidden (height of 0.0) and the NTP manages its own internal toolbars. This CL uses the corrected viewport insets in order to calculate the visible portions of the content area in order to lay out the NTP and specify the inset to use for native controllers. The referenced bug occurred because BVC's old implementation of |-nativeContentHeaderHeightForWebState:| returned the header height, which caused the native content to be mistakenly pushed down despite the fact that WebState::GetView() was already laid out below the toolbars on iOS 11. TBR=kkhorimoto@chromium.org (cherry picked from commit a36505e85442fa4224253dad25a05f979611ef44) Bug: 902271 Change-Id: I364773e74b3d9d0ec7b433de5462666f21b1de5b Reviewed-on: https://chromium-review.googlesource.com/c/1378847 Commit-Queue: Kurt Horimoto <kkhorimoto@chromium.org> Reviewed-by: Eugene But <eugenebut@chromium.org> Reviewed-by: Gauthier Ambard <gambard@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#618727} Reviewed-on: https://chromium-review.googlesource.com/c/1395810 Cr-Commit-Position: refs/branch-heads/3626@{#568} Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437} [modify] https://crrev.com/088b5a224a03b25700959dcf9a97ad9ddc46182b/ios/chrome/browser/prerender/preload_controller.mm [modify] https://crrev.com/088b5a224a03b25700959dcf9a97ad9ddc46182b/ios/chrome/browser/prerender/preload_controller_delegate.h [modify] https://crrev.com/088b5a224a03b25700959dcf9a97ad9ddc46182b/ios/chrome/browser/ui/browser_view_controller.mm [modify] https://crrev.com/088b5a224a03b25700959dcf9a97ad9ddc46182b/ios/web/public/test/fakes/test_native_content_provider.mm [modify] https://crrev.com/088b5a224a03b25700959dcf9a97ad9ddc46182b/ios/web/public/web_state/ui/crw_native_content_provider.h [modify] https://crrev.com/088b5a224a03b25700959dcf9a97ad9ddc46182b/ios/web/web_state/ui/crw_web_controller.mm [modify] https://crrev.com/088b5a224a03b25700959dcf9a97ad9ddc46182b/ios/web/web_state/ui/crw_web_controller_container_view.h [modify] https://crrev.com/088b5a224a03b25700959dcf9a97ad9ddc46182b/ios/web/web_state/ui/crw_web_controller_container_view.mm
,
Jan 7
The cherry-pick from c#22 uses API that was introduced after M72 branched. This API was added in crrev.com/c/1339460, so requesting merge for that CL as well. Even though that CL looks larger because it touches several files, it's mostly just moving an inset calculation from the resizer to the model. The rest of the CL is just plumbing code to expose that calculation through FullscreenController.
,
Jan 7
,
Jan 7
This bug requires manual review: Less than 18 days to go before AppStore submit on M72 Please contact the milestone owner if you have questions. Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jan 7
,
Jan 7
CL is merged. The bugdroid comment appeared on Issue 674649, so I'm updating manually.
,
Jan 9
Verified in: App Version: 72.0.3626.51 beta Devices: iPhone 6, iPhone XS Max, iPad Mini iOS Versions: 11.4.1, 12.1.2, 12.1.3 beta 3 External PDF file loads successfully in chrome |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by sczs@chromium.org
, Nov 7Owner: mrefaat@chromium.org
Status: Assigned (was: Untriaged)