Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2599643002:
Fix reading PDF images from NSPasteboard. (Closed)
|
Created:
4 years ago by erikchen Modified:
4 years ago Reviewers:
Avi (use Gerrit) CC:
chromium-reviews, dcheng, mac-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix reading PDF images from NSPasteboard.
PDF images have a single representation [NSPDFImageRep] with a size but not
pixelSize. The previous code assumes that pixelSize would always be valid.
BUG=676163
Committed: https://crrev.com/9597b569111b84dd7d96c04644e75783c6211547
Cr-Commit-Position: refs/heads/master@{#440458}
Patch Set 1 #Patch Set 2 : Cleanup. #
Total comments: 4
Patch Set 3 : Comments from avi. #
Messages
Total messages: 20 (10 generated)
The CQ bit was checked by erikchen@chromium.org to run a CQ dry run
erikchen@chromium.org changed reviewers: + avi@chromium.org
avi: Please review.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2599643002/diff/20001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_mac.mm (right): https://codereview.chromium.org/2599643002/diff/20001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_mac.mm:343: image.get(), /*is_opaque=*/false, base::mac::GetSystemColorSpace()); Wait... why don't we always go this path? Why do we extract out the image rep for that other call?
https://codereview.chromium.org/2599643002/diff/20001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_mac.mm (right): https://codereview.chromium.org/2599643002/diff/20001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_mac.mm:343: image.get(), /*is_opaque=*/false, base::mac::GetSystemColorSpace()); On 2016/12/22 03:22:03, Avi wrote: > Wait... why don't we always go this path? Why do we extract out the image rep > for that other call? this loses pixels from retina images: https://codereview.chromium.org/2276183002
lgtm https://codereview.chromium.org/2599643002/diff/20001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_mac.mm (right): https://codereview.chromium.org/2599643002/diff/20001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_mac.mm:343: image.get(), /*is_opaque=*/false, base::mac::GetSystemColorSpace()); On 2016/12/22 04:51:49, erikchen wrote: > On 2016/12/22 03:22:03, Avi wrote: > > Wait... why don't we always go this path? Why do we extract out the image rep > > for that other call? > > this loses pixels from retina images: > https://codereview.chromium.org/2276183002 Can you mention this in a comment?
On 2016/12/22 05:42:25, Avi wrote: > lgtm > > https://codereview.chromium.org/2599643002/diff/20001/ui/base/clipboard/clipb... > File ui/base/clipboard/clipboard_mac.mm (right): > > https://codereview.chromium.org/2599643002/diff/20001/ui/base/clipboard/clipb... > ui/base/clipboard/clipboard_mac.mm:343: image.get(), /*is_opaque=*/false, > base::mac::GetSystemColorSpace()); > On 2016/12/22 04:51:49, erikchen wrote: > > On 2016/12/22 03:22:03, Avi wrote: > > > Wait... why don't we always go this path? Why do we extract out the image > rep > > > for that other call? > > > > this loses pixels from retina images: > > https://codereview.chromium.org/2276183002 > > Can you mention this in a comment? Wait. Why don't we roll all of this special handling into NSImageToSkBitmapWithColorSpace? Shouldn't all callers of that function properly handle retina images?
https://codereview.chromium.org/2599643002/diff/20001/ui/base/clipboard/clipb... File ui/base/clipboard/clipboard_mac.mm (right): https://codereview.chromium.org/2599643002/diff/20001/ui/base/clipboard/clipb... ui/base/clipboard/clipboard_mac.mm:343: image.get(), /*is_opaque=*/false, base::mac::GetSystemColorSpace()); On 2016/12/22 05:42:25, Avi wrote: > On 2016/12/22 04:51:49, erikchen wrote: > > On 2016/12/22 03:22:03, Avi wrote: > > > Wait... why don't we always go this path? Why do we extract out the image > rep > > > for that other call? > > > > this loses pixels from retina images: > > https://codereview.chromium.org/2276183002 > > Can you mention this in a comment? Done.
> > Wait. Why don't we roll all of this special handling into > NSImageToSkBitmapWithColorSpace? Shouldn't all callers of that function properly > handle retina images? This is the only caller. Note that regardless of what we do, we are losing information. [display size vs pixel size]. So the only question is where we choose to lose this information. clipboard_mac seems like a reasonable place to lose this information, so that we don't need to expose Skia to the internals of how NSImage works [beyond the basic - let's grab some pixels].
The CQ bit was checked by erikchen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org Link to the patchset: https://codereview.chromium.org/2599643002/#ps40001 (title: "Comments from avi.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1482431031250250,
"parent_rev": "d8f7277852d81e7f5b85ac56c9b6614b2ba2a8a5", "commit_rev":
"ab2f32a5504f569726f3b85b61b88bc095519190"}
Message was sent while issue was closed.
Description was changed from ========== Fix reading PDF images from NSPasteboard. PDF images have a single representation [NSPDFImageRep] with a size but not pixelSize. The previous code assumes that pixelSize would always be valid. BUG=676163 ========== to ========== Fix reading PDF images from NSPasteboard. PDF images have a single representation [NSPDFImageRep] with a size but not pixelSize. The previous code assumes that pixelSize would always be valid. BUG=676163 Review-Url: https://codereview.chromium.org/2599643002 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Fix reading PDF images from NSPasteboard. PDF images have a single representation [NSPDFImageRep] with a size but not pixelSize. The previous code assumes that pixelSize would always be valid. BUG=676163 Review-Url: https://codereview.chromium.org/2599643002 ========== to ========== Fix reading PDF images from NSPasteboard. PDF images have a single representation [NSPDFImageRep] with a size but not pixelSize. The previous code assumes that pixelSize would always be valid. BUG=676163 Committed: https://crrev.com/9597b569111b84dd7d96c04644e75783c6211547 Cr-Commit-Position: refs/heads/master@{#440458} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/9597b569111b84dd7d96c04644e75783c6211547 Cr-Commit-Position: refs/heads/master@{#440458} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
