fix(favicon): address pull request review feedback
- commit
- 88cfbd5346d1ae5631f693bb5e7fed2128175bc8
- author
- brice · · 2026-06-07 18:41 UTC
- parent
- 8305e33
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
5 files · +24 -9
M internal/application/usecase/favicon.go +3 -3
| @@ -209,7 +209,7 @@ func (uc *FaviconUseCase) Observe( | |||
| 209 | 209 | var converted *appport.ConvertedFavicon | |
| 210 | 210 | if uc.converter != nil { | |
| 211 | 211 | var err error | |
| 212 | - | converted, err = uc.converter.Convert(ctx, bytes, contentType, []int{32}) | |
| 212 | + | converted, err = uc.converter.Convert(ctx, bytes, contentType, []int{SystemviewIconSize}) | |
| 213 | 213 | if err != nil { | |
| 214 | 214 | return nil, err | |
| 215 | 215 | } | |
| @@ -227,8 +227,8 @@ func (uc *FaviconUseCase) Observe( | |||
| 227 | 227 | return nil, err | |
| 228 | 228 | } | |
| 229 | 229 | } | |
| 230 | - | if b := converted.SizedPNG[32]; len(b) > 0 { | |
| 231 | - | if err := uc.blobs.WriteSizedPNG(ctx, key, 32, b); err != nil { | |
| 230 | + | if b := converted.SizedPNG[SystemviewIconSize]; len(b) > 0 { | |
| 231 | + | if err := uc.blobs.WriteSizedPNG(ctx, key, SystemviewIconSize, b); err != nil { | |
| 232 | 232 | return nil, err | |
| 233 | 233 | } | |
| 234 | 234 | } | |
| @@ -209,7 +209,7 @@ func (uc *FaviconUseCase) Observe( | |||||
| 209 | var converted *appport.ConvertedFavicon | 209 | var converted *appport.ConvertedFavicon | ||
| 210 | if uc.converter != nil { | 210 | if uc.converter != nil { | ||
| 211 | var err error | 211 | var err error | ||
| 212 | - | converted, err = uc.converter.Convert(ctx, bytes, contentType, []int{32}) | 212 | + | converted, err = uc.converter.Convert(ctx, bytes, contentType, []int{SystemviewIconSize}) |
| 213 | if err != nil { | 213 | if err != nil { | ||
| 214 | return nil, err | 214 | return nil, err | ||
| 215 | } | 215 | } | ||
| @@ -227,8 +227,8 @@ func (uc *FaviconUseCase) Observe( | |||||
| 227 | return nil, err | 227 | return nil, err | ||
| 228 | } | 228 | } | ||
| 229 | } | 229 | } | ||
| 230 | - | if b := converted.SizedPNG[32]; len(b) > 0 { | 230 | + | if b := converted.SizedPNG[SystemviewIconSize]; len(b) > 0 { |
| 231 | - | if err := uc.blobs.WriteSizedPNG(ctx, key, 32, b); err != nil { | 231 | + | if err := uc.blobs.WriteSizedPNG(ctx, key, SystemviewIconSize, b); err != nil { |
| 232 | return nil, err | 232 | return nil, err | ||
| 233 | } | 233 | } | ||
| 234 | } | 234 | } | ||
M internal/infrastructure/cef/scheme_handler.go +13 -2
| @@ -837,8 +837,10 @@ func newFaviconResourceHandler( | |||
| 837 | 837 | domain string, | |
| 838 | 838 | size int, | |
| 839 | 839 | ) purecef.ResourceHandler { | |
| 840 | + | reqCtx, cancel := context.WithCancel(ctx) //nolint:gosec // canceled by load completion or CEF Cancel callback | |
| 840 | 841 | return cefNewResourceHandler(&faviconResourceHandler{ | |
| 841 | - | ctx: ctx, | |
| 842 | + | ctx: reqCtx, | |
| 843 | + | cancel: cancel, | |
| 842 | 844 | resolver: resolver, | |
| 843 | 845 | domain: domain, | |
| 844 | 846 | size: size, | |
| @@ -849,6 +851,8 @@ func newFaviconResourceHandler( | |||
| 849 | 851 | ||
| 850 | 852 | type faviconResourceHandler struct { | |
| 851 | 853 | ctx context.Context | |
| 854 | + | cancel context.CancelFunc | |
| 855 | + | cancelOnce sync.Once | |
| 852 | 856 | resolver port.FaviconSystemviewResolver | |
| 853 | 857 | domain string | |
| 854 | 858 | size int | |
| @@ -863,6 +867,7 @@ type faviconResourceHandler struct { | |||
| 863 | 867 | ||
| 864 | 868 | func (rh *faviconResourceHandler) load() { | |
| 865 | 869 | defer close(rh.done) | |
| 870 | + | defer rh.cancelRequest() | |
| 866 | 871 | rh.statusCode = http.StatusNotFound | |
| 867 | 872 | rh.contentType = "application/json" | |
| 868 | 873 | rh.data = []byte(`{"error":"favicon not cached"}`) | |
| @@ -954,7 +959,13 @@ func (rh *faviconResourceHandler) ReadResponse(_ unsafe.Pointer, _ int32, _ *int | |||
| 954 | 959 | return 0 | |
| 955 | 960 | } | |
| 956 | 961 | ||
| 957 | - | func (rh *faviconResourceHandler) Cancel() {} | |
| 962 | + | func (rh *faviconResourceHandler) Cancel() { rh.cancelRequest() } | |
| 963 | + | ||
| 964 | + | func (rh *faviconResourceHandler) cancelRequest() { | |
| 965 | + | if rh.cancel != nil { | |
| 966 | + | rh.cancelOnce.Do(rh.cancel) | |
| 967 | + | } | |
| 968 | + | } | |
| 958 | 969 | ||
| 959 | 970 | // Open handles the request immediately (synchronous). | |
| 960 | 971 | func (rh *staticResourceHandler) Open(_ purecef.Request, handleRequest *int32, _ purecef.Callback) int32 { | |
| @@ -837,8 +837,10 @@ func newFaviconResourceHandler( | |||||
| 837 | domain string, | 837 | domain string, | ||
| 838 | size int, | 838 | size int, | ||
| 839 | ) purecef.ResourceHandler { | 839 | ) purecef.ResourceHandler { | ||
| 840 | + | reqCtx, cancel := context.WithCancel(ctx) //nolint:gosec // canceled by load completion or CEF Cancel callback | |||
| 840 | return cefNewResourceHandler(&faviconResourceHandler{ | 841 | return cefNewResourceHandler(&faviconResourceHandler{ | ||
| 841 | - | ctx: ctx, | 842 | + | ctx: reqCtx, |
| 843 | + | cancel: cancel, | |||
| 842 | resolver: resolver, | 844 | resolver: resolver, | ||
| 843 | domain: domain, | 845 | domain: domain, | ||
| 844 | size: size, | 846 | size: size, | ||
| @@ -849,6 +851,8 @@ func newFaviconResourceHandler( | |||||
| 849 | 851 | ||||
| 850 | type faviconResourceHandler struct { | 852 | type faviconResourceHandler struct { | ||
| 851 | ctx context.Context | 853 | ctx context.Context | ||
| 854 | + | cancel context.CancelFunc | |||
| 855 | + | cancelOnce sync.Once | |||
| 852 | resolver port.FaviconSystemviewResolver | 856 | resolver port.FaviconSystemviewResolver | ||
| 853 | domain string | 857 | domain string | ||
| 854 | size int | 858 | size int | ||
| @@ -863,6 +867,7 @@ type faviconResourceHandler struct { | |||||
| 863 | 867 | ||||
| 864 | func (rh *faviconResourceHandler) load() { | 868 | func (rh *faviconResourceHandler) load() { | ||
| 865 | defer close(rh.done) | 869 | defer close(rh.done) | ||
| 870 | + | defer rh.cancelRequest() | |||
| 866 | rh.statusCode = http.StatusNotFound | 871 | rh.statusCode = http.StatusNotFound | ||
| 867 | rh.contentType = "application/json" | 872 | rh.contentType = "application/json" | ||
| 868 | rh.data = []byte(`{"error":"favicon not cached"}`) | 873 | rh.data = []byte(`{"error":"favicon not cached"}`) | ||
| @@ -954,7 +959,13 @@ func (rh *faviconResourceHandler) ReadResponse(_ unsafe.Pointer, _ int32, _ *int | |||||
| 954 | return 0 | 959 | return 0 | ||
| 955 | } | 960 | } | ||
| 956 | 961 | ||||
| 957 | - | func (rh *faviconResourceHandler) Cancel() {} | 962 | + | func (rh *faviconResourceHandler) Cancel() { rh.cancelRequest() } |
| 963 | + | ||||
| 964 | + | func (rh *faviconResourceHandler) cancelRequest() { | |||
| 965 | + | if rh.cancel != nil { | |||
| 966 | + | rh.cancelOnce.Do(rh.cancel) | |||
| 967 | + | } | |||
| 968 | + | } | |||
| 958 | 969 | ||||
| 959 | // Open handles the request immediately (synchronous). | 970 | // Open handles the request immediately (synchronous). | ||
| 960 | func (rh *staticResourceHandler) Open(_ purecef.Request, handleRequest *int32, _ purecef.Callback) int32 { | 971 | func (rh *staticResourceHandler) Open(_ purecef.Request, handleRequest *int32, _ purecef.Callback) int32 { | ||
M internal/infrastructure/cef/webview_handlers.go +1 -0
| @@ -388,6 +388,7 @@ func (wv *WebView) retainFaviconSourceVisitor(visitor purecef.StringVisitor) fun | |||
| 388 | 388 | defer wv.faviconSourceVisitorsMu.Unlock() | |
| 389 | 389 | for i, candidate := range wv.faviconSourceVisitors { | |
| 390 | 390 | if candidate.id == id { | |
| 391 | + | wv.faviconSourceVisitors[i].visitor = nil | |
| 391 | 392 | wv.faviconSourceVisitors = append(wv.faviconSourceVisitors[:i], wv.faviconSourceVisitors[i+1:]...) | |
| 392 | 393 | return | |
| 393 | 394 | } | |
| @@ -388,6 +388,7 @@ func (wv *WebView) retainFaviconSourceVisitor(visitor purecef.StringVisitor) fun | |||||
| 388 | defer wv.faviconSourceVisitorsMu.Unlock() | 388 | defer wv.faviconSourceVisitorsMu.Unlock() | ||
| 389 | for i, candidate := range wv.faviconSourceVisitors { | 389 | for i, candidate := range wv.faviconSourceVisitors { | ||
| 390 | if candidate.id == id { | 390 | if candidate.id == id { | ||
| 391 | + | wv.faviconSourceVisitors[i].visitor = nil | |||
| 391 | wv.faviconSourceVisitors = append(wv.faviconSourceVisitors[:i], wv.faviconSourceVisitors[i+1:]...) | 392 | wv.faviconSourceVisitors = append(wv.faviconSourceVisitors[:i], wv.faviconSourceVisitors[i+1:]...) | ||
| 392 | return | 393 | return | ||
| 393 | } | 394 | } | ||
M internal/infrastructure/favicon/service.go +0 -3
| @@ -29,9 +29,6 @@ func NewService(cacheDir string) *Service { | |||
| 29 | 29 | } | |
| 30 | 30 | ||
| 31 | 31 | func (s *Service) Cache() *Cache { | |
| 32 | - | if s == nil { | |
| 33 | - | return nil | |
| 34 | - | } | |
| 35 | 32 | return s.cache | |
| 36 | 33 | } | |
| 37 | 34 | ||
| @@ -29,9 +29,6 @@ func NewService(cacheDir string) *Service { | |||||
| 29 | } | 29 | } | ||
| 30 | 30 | ||||
| 31 | func (s *Service) Cache() *Cache { | 31 | func (s *Service) Cache() *Cache { | ||
| 32 | - | if s == nil { | |||
| 33 | - | return nil | |||
| 34 | - | } | |||
| 35 | return s.cache | 32 | return s.cache | ||
| 36 | } | 33 | } | ||
| 37 | 34 | ||||
M internal/infrastructure/webkit/scheme_handler.go +7 -1
| @@ -71,6 +71,7 @@ func (f PageHandlerFunc) Handle(req *SchemeRequest) *SchemeResponse { | |||
| 71 | 71 | ||
| 72 | 72 | // DumbSchemeHandler handles dumb:// URI scheme requests. | |
| 73 | 73 | type DumbSchemeHandler struct { | |
| 74 | + | ctx context.Context | |
| 74 | 75 | handlers map[string]PageHandler | |
| 75 | 76 | assets embed.FS | |
| 76 | 77 | faviconResolver port.FaviconSystemviewResolver | |
| @@ -86,6 +87,7 @@ func NewDumbSchemeHandler(ctx context.Context) *DumbSchemeHandler { | |||
| 86 | 87 | log := logging.FromContext(ctx) | |
| 87 | 88 | ||
| 88 | 89 | h := &DumbSchemeHandler{ | |
| 90 | + | ctx: ctx, | |
| 89 | 91 | handlers: make(map[string]PageHandler), | |
| 90 | 92 | assetDir: systemviewsAssetDir, | |
| 91 | 93 | logger: log.With().Str("component", "scheme-handler").Logger(), | |
| @@ -233,7 +235,11 @@ func (h *DumbSchemeHandler) handleFaviconAPI(req *SchemeRequest) *SchemeResponse | |||
| 233 | 235 | size = parsedSize | |
| 234 | 236 | } | |
| 235 | 237 | ||
| 236 | - | resolved, err := resolver.ResolveSystemviewIcon(context.Background(), domain, size) | |
| 238 | + | resolveCtx := h.ctx | |
| 239 | + | if resolveCtx == nil { | |
| 240 | + | resolveCtx = context.Background() | |
| 241 | + | } | |
| 242 | + | resolved, err := resolver.ResolveSystemviewIcon(resolveCtx, domain, size) | |
| 237 | 243 | if err != nil || resolved == nil || len(resolved.Bytes) == 0 { | |
| 238 | 244 | return privateJSONErrorResponse(http.StatusNotFound, "favicon not cached") | |
| 239 | 245 | } | |
| @@ -71,6 +71,7 @@ func (f PageHandlerFunc) Handle(req *SchemeRequest) *SchemeResponse { | |||||
| 71 | 71 | ||||
| 72 | // DumbSchemeHandler handles dumb:// URI scheme requests. | 72 | // DumbSchemeHandler handles dumb:// URI scheme requests. | ||
| 73 | type DumbSchemeHandler struct { | 73 | type DumbSchemeHandler struct { | ||
| 74 | + | ctx context.Context | |||
| 74 | handlers map[string]PageHandler | 75 | handlers map[string]PageHandler | ||
| 75 | assets embed.FS | 76 | assets embed.FS | ||
| 76 | faviconResolver port.FaviconSystemviewResolver | 77 | faviconResolver port.FaviconSystemviewResolver | ||
| @@ -86,6 +87,7 @@ func NewDumbSchemeHandler(ctx context.Context) *DumbSchemeHandler { | |||||
| 86 | log := logging.FromContext(ctx) | 87 | log := logging.FromContext(ctx) | ||
| 87 | 88 | ||||
| 88 | h := &DumbSchemeHandler{ | 89 | h := &DumbSchemeHandler{ | ||
| 90 | + | ctx: ctx, | |||
| 89 | handlers: make(map[string]PageHandler), | 91 | handlers: make(map[string]PageHandler), | ||
| 90 | assetDir: systemviewsAssetDir, | 92 | assetDir: systemviewsAssetDir, | ||
| 91 | logger: log.With().Str("component", "scheme-handler").Logger(), | 93 | logger: log.With().Str("component", "scheme-handler").Logger(), | ||
| @@ -233,7 +235,11 @@ func (h *DumbSchemeHandler) handleFaviconAPI(req *SchemeRequest) *SchemeResponse | |||||
| 233 | size = parsedSize | 235 | size = parsedSize | ||
| 234 | } | 236 | } | ||
| 235 | 237 | ||||
| 236 | - | resolved, err := resolver.ResolveSystemviewIcon(context.Background(), domain, size) | 238 | + | resolveCtx := h.ctx |
| 239 | + | if resolveCtx == nil { | |||
| 240 | + | resolveCtx = context.Background() | |||
| 241 | + | } | |||
| 242 | + | resolved, err := resolver.ResolveSystemviewIcon(resolveCtx, domain, size) | |||
| 237 | if err != nil || resolved == nil || len(resolved.Bytes) == 0 { | 243 | if err != nil || resolved == nil || len(resolved.Bytes) == 0 { | ||
| 238 | return privateJSONErrorResponse(http.StatusNotFound, "favicon not cached") | 244 | return privateJSONErrorResponse(http.StatusNotFound, "favicon not cached") | ||
| 239 | } | 245 | } | ||